Add OSS statistics/monitoring plugin #2279

bbockelm · 2024-05-31T13:04:11Z

This PR adds a new OSS implementation to XRootD for gathering statistics about the underlying OSS performance.

It keeps counters about the number of operations performed and the overall time spent performing these operations then sends them out to the g-stream. It keeps a separate set of counters for "slow" operations (configurable), providing insight to administrators about the user experience beyond the averages. The intent is this information can easily be passed to time-series monitoring system such as Prometheus.

This adds two configuration options:

fsstats.trace [all|debug|info|warning|error]

fsstats.trace manages the verbosity of the logging done by the OSS statistics library. When debug
information is enabled, it will print out the performance counters once per second.

If fsstats.trace is not specified, then the default is fsstats.trace warning.

and

fsstats.slowops [duration]

The fsstats.slowops directive determines the threshold where a given storage operation is
considered "slow". Slow operations are kept as a separate set of monitoring counters, allowing
the administrator to see how often a storage system has poor performance.

The duration must be specified as a number with a unit such as 1.5s or 1s50ms; valid units
include h (hours), m (minutes), s (seconds), ms (milliseconds), us (microseconds), or
ns (nanoseconds).

If fsstats.slowops is not specified, then the default is fsstats.slowops 1s.

While the intent is the implementation is lightweight statistics gathering, everything has a cost; hence, this is kept as a loadable plugin instead of in the XRootD core.

The initial use case is to understand the performance of the storage subsystems in XCache. Currently our only performance monitoring is from looking at the throttle plugin. The throttle plugin includes both the storage performance for cache hits and the proxy performance for cache misses; I want to exclude the latter.

amadio · 2024-05-31T14:04:11Z

I've requested a review from @abh3, but first the CI needs to be fully green. You can look at the jobs matching this pull request number in CDash for a quicker way to dig through the error logs of the builds. Cheers,

abh3 · 2024-05-31T21:50:16Z

Sure, I'll review it but it will take me a bit longer than normal as I will be busy with an ATLAS meeting in Oslo.

amadio · 2024-06-01T11:38:23Z

Sure, I'll review it but it will take me a bit longer than normal as I will be busy with an ATLAS meeting in Oslo.

This needs a major release, I believe, so no need to rush. Cheers,

abh3

So I am confused as to what this is. A real storage system or a wrapper to a storage system. My inclination is that this is a wrapper and, if so, i9t does not follow the conventions for wrapping an oss plugin. First, it should use the class defined in XrdOssWrapper.hh because doing so alleviates us from having to chase down yet another plugin API when we make future changes in the Oss API. Second, as a wrapper it should not have a GetFileSystem entry point as it can only be added using the "++" convention. Thirdly, a wrapper should not load or otherwise initialize the file system it is wrapping. That is done by the plugin system. So, that code is not appropriate here. So, could this be rewritten to follow the standard way Oss plugins wrappers should be written?

bbockelm · 2024-06-17T16:43:48Z

First, it should use the class defined in XrdOssWrapper.hh because doing so alleviates us from having to chase down yet another plugin API when we make future changes in the Oss API

Are there any examples of using these classes? I looked at the wrapper and it appears simple enough but it'd be good to know if I'm the only user of the class.

Second, as a wrapper it should not have a GetFileSystem entry point as it can only be added using the "++" convention. Thirdly, a wrapper should not load or otherwise initialize the file system it is wrapping.

Sadly, it's not possible to rely on the ++ syntax as you suggest due to #1730. Unfortunately, that issue means a wrapper writer needs to implement exactly what you say the wrapper should not do. If you are able tackle issue #1730, then I'd happily drop the other entry points (on this and other plugins).

abh3 · 2024-06-17T18:08:57Z

Hi Brian, No, you are not the only user. We have an upcoming tape RSE plugin, and we will be converting existing wrapper plugins into using the official wrapper. Andy

…

________________________________ From: Brian P Bockelman ***@***.***> Sent: Monday, June 17, 2024 9:44 AM To: xrootd/xrootd ***@***.***> Cc: Andrew Hanushevsky ***@***.***>; Mention ***@***.***> Subject: Re: [xrootd/xrootd] Add OSS statistics/monitoring plugin (PR #2279) First, it should use the class defined in XrdOssWrapper.hh because doing so alleviates us from having to chase down yet another plugin API when we make future changes in the Oss API Are there any examples of using these classes? I looked at the wrapper and it appears simple enough but it'd be good to know if I'm the only user of the class. Second, as a wrapper it should not have a GetFileSystem entry point as it can only be added using the "++" convention. Thirdly, a wrapper should not load or otherwise initialize the file system it is wrapping. Sadly, it's not possible to rely on the ++ syntax as you suggest due to #1730<#1730>. Unfortunately, that issue means a wrapper writer needs to implement exactly what you say the wrapper should not do. If you are able tackle issue #1730<#1730>, then I'd happily drop the other entry points (on this and other plugins). — Reply to this email directly, view it on GitHub<#2279 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUIW56VX26PN4XMLSXXBDDZH4G5TAVCNFSM6AAAAABISZAPQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTHA3TIMRYGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

abh3 · 2024-06-17T18:12:46Z

As for 1730 that is a cmsd problem and as outlined in the issue, the only thing the cmsd wants to use is stat() which itself is replaceable via the stat plugin. So practically there is no reason to stack the cmsd plugin. I suspect this plugin would not be used for the cmsd as well. So, I think you can ignore issue 1730. I agree that the cmsd should not be silent in this case.

…

________________________________ From: Brian P Bockelman ***@***.***> Sent: Monday, June 17, 2024 9:44 AM To: xrootd/xrootd ***@***.***> Cc: Andrew Hanushevsky ***@***.***>; Mention ***@***.***> Subject: Re: [xrootd/xrootd] Add OSS statistics/monitoring plugin (PR #2279) First, it should use the class defined in XrdOssWrapper.hh because doing so alleviates us from having to chase down yet another plugin API when we make future changes in the Oss API Are there any examples of using these classes? I looked at the wrapper and it appears simple enough but it'd be good to know if I'm the only user of the class. Second, as a wrapper it should not have a GetFileSystem entry point as it can only be added using the "++" convention. Thirdly, a wrapper should not load or otherwise initialize the file system it is wrapping. Sadly, it's not possible to rely on the ++ syntax as you suggest due to #1730<#1730>. Unfortunately, that issue means a wrapper writer needs to implement exactly what you say the wrapper should not do. If you are able tackle issue #1730<#1730>, then I'd happily drop the other entry points (on this and other plugins). — Reply to this email directly, view it on GitHub<#2279 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUIW56VX26PN4XMLSXXBDDZH4G5TAVCNFSM6AAAAABISZAPQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZTHA3TIMRYGM>. You are receiving this because you were mentioned.Message ID: ***@***.***>

bbockelm · 2024-07-24T13:11:19Z

@abh3 - updated to rebase on the XrdOssWrapper and removed the support for running as part of the cmsd.

Is the XrdOssWrapper part of the public API/ABI yet? The design is awkward to utilize because it does not own the wrapDF object. Hence, I have to keep two copies of the pointer, once in StatsFile (to own the lifetime) and once in the XrdOssWrapDF (a reference).

If the API is not frozen, it'd be better to change the wrapper to own the object to manage the lifetime.

xrootd-dev · 2024-07-24T22:50:52Z

Yes, it is part of the public API. Yes, you cannot control the lifetime of Oss system object because you should be using the passed pointer whose lifetime you do not control. However, you are correct that the lifetime of the DF object is something you do control. We need to wait for R6 to change that. Unfortunately, keeping a separate pointer is the only way to handle the lifetime situation for now.

…

________________________________ From: ***@***.*** ***@***.***> on behalf of Brian P Bockelman ***@***.***> Sent: Wednesday, July 24, 2024 6:11 AM To: xrootd/xrootd ***@***.***> Cc: Subscribed ***@***.***> Subject: Re: [xrootd/xrootd] Add OSS statistics/monitoring plugin (PR #2279) @abh3<https://github.com/abh3> - updated to rebase on the XrdOssWrapper and removed the support for running as part of the cmsd. Is the XrdOssWrapper part of the public API/ABI yet? The design is awkward to utilize because it does not own the wrapDF object. Hence, I have to keep two copies of the pointer, once in StatsFile (to own the lifetime) and once in the XrdOssWrapDF (a reference). If the API is not frozen, it'd be better to change the wrapper to own the object to manage the lifetime. — Reply to this email directly, view it on GitHub<#2279 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA7NRDUPNDMMMWXOKFCK7C3ZN6RZBAVCNFSM6AAAAABISZAPQGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENBXHA4DONRSGA>. You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

________________________________ Use REPLY-ALL to reply to list To unsubscribe from the XROOTD-DEV list, click the following link: http://listserv.slac.stanford.edu/scripts/wa.exe?SUBED1=XROOTD-DEV&A=1

bbockelm · 2024-08-26T17:54:08Z

@abh3 - just a bump on this PR (I see I didn't press the "re-review" button; I'll do that now). If it wasn't obvious from the history, I did the refactoring you requested back in July. My follow-up questions were simply around the API design.

abh3 · 2024-08-26T19:04:19Z

Will do, it's a bit hectic at the moment with the workshop less than 2 weeks away.

…

________________________________ From: Brian P Bockelman ***@***.***> Sent: Monday, August 26, 2024 10:54 AM To: xrootd/xrootd ***@***.***> Cc: Andrew Hanushevsky ***@***.***>; Review requested ***@***.***> Subject: Re: [xrootd/xrootd] Add OSS statistics/monitoring plugin (PR #2279) BEWARE: This email originated outside of our organization. DO NOT CLICK links or attachments unless you recognize the sender and know the content is safe. @bbockelm<https://github.com/bbockelm> requested your review on: #2279<#2279> Add OSS statistics/monitoring plugin. — Reply to this email directly, view it on GitHub<#2279 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAUIW52DQ57AU7GPYDGUPC3ZTNTVRAVCNFSM6AAAAABISZAPQGVHI2DSMVQWIX3LMV45UABCJFZXG5LFIV3GK3TUJZXXI2LGNFRWC5DJN5XDWMJUGAYTMOJVGA3TAMQ>. You are receiving this because your review was requested.Message ID: ***@***.***>

abh3

OK, let's resolve the comments.

src/XrdOssStats/XrdStatsFileSystem.hh

src/XrdPlugins.cmake

src/XrdOssStats/XrdStatsConfig.cc

src/XrdOssStats/XrdStatsFileSystem.cc

bbockelm · 2025-01-27T14:13:37Z

@abh3 - any news on this front?

I left the one remaining discussion thread open because I wanted to make sure you understood how the visibility changes work.

amadio · 2025-01-27T14:53:13Z

This needs to wait for a feature release (i.e. at least 5.8), while the next one will be a patch release (i.e. 5.7.3). Other than that, once @abh3 approves, we can merge to devel.

bbockelm · 2025-01-27T15:52:43Z

Yes, no problems waiting for a feature release! Just trying to tidy up pending PRs on a Monday morning.

abh3

@bbockelm Almost done here. Only one question remains (no code changes needed). We don'y like putting things in the repo that a) are not explainable, b) look like they are not needed. So, could you address the question posed here.

abh3 · 2025-02-06T13:02:51Z

src/XrdOssStats/export-lib-symbols

@@ -0,0 +1,7 @@
+{


Could you explain why this is needed? By default, the way XrdOssAddStorageSystem2 has been declared automatically makes it visible. Any other symbols are visible when made extern, making everything visible is generally not good practice.

You might be reading this backward. This makes all symbols local to the library (not appearing in the symbol table) except for XrdOssAddStorageSystem2*. If I didn't have the exception listed in the global section, then it would hide XrdOssAddStorageSystem2* as well -- not what we want!

Any other symbols are visible when made extern, making everything visible is generally not good practice.

That's not how C++ works. The extern keyword specifies the storage class for the variable or the language linkage property for functions. It does not affect symbol visibility in the resulting module.

For example, consider the SciTokens library (which doesn't manage symbol visibility:

$ nm -D lib64/libXrdAccSciTokens-5.so | c++filt | grep -v ' U ' | wc -l 176

Quite a bit of "stuff" -- all not needed to be visible from the outside because, really, all that's needed is XrdAccAuthorizeObjAdd and friends. Loading it pollutes up the global symbol table.

This is not because extern is or is not used -- but because C++ exports all the namespace-level symbols globally by default when dynamic linking.

Compare this to libXrdMacaroons where we do manage the symbol visibility:

$ nm -D lib64/libXrdMacaroons-5.so | c++filt | grep -v ' U ' | wc -l 14

The list is short enough I can paste it into the comment -- and it looks reasonable:

$ nm -D build/release_dir/lib64/libXrdMacaroons-5.so | c++filt | grep -v ' U ' w __cxa_finalize 000000000000e678 t _fini w __gmon_start__ 0000000000004000 t _init w _ITM_deregisterTMCloneTable w _ITM_registerTMCloneTable w __pthread_key_create 0000000000014618 B SciTokensHelper 0000000000005be0 T XrdAccAuthorizeObjAdd 0000000000014540 D XrdAccAuthorizeObjAdd_ 0000000000005c50 T XrdAccAuthorizeObject 00000000000145a0 D XrdAccAuthorizeObject_ 0000000000005f20 T XrdHttpGetExtHandler 00000000000144e0 D XrdHttpGetExtHandler_

You only have things that are desired to be exported -- and basically required for a shared library (_init, _fini).

Most of the Xrootd modules export way more than intended: based on your review comment, I wanted to tidy up this one.

abh3

You are correct, I read it backwards. Thanks for the explanation.

amadio · 2025-02-07T12:22:31Z

src/XrdOssStats/XrdOssStatsFileSystem.cc

+        static_cast<float>(m_slow_times.m_open)/1e9, static_cast<float>(m_slow_times.m_read)/1e9, static_cast<float>(m_slow_times.m_readv)/1e9,
+        static_cast<float>(m_slow_times.m_pgread)/1e9, static_cast<float>(m_slow_times.m_write)/1e9, static_cast<float>(m_slow_times.m_pgwrite)/1e9,
+        static_cast<float>(m_slow_times.m_dirlist)/1e9, static_cast<float>(m_slow_times.m_stat)/1e9, static_cast<float>(m_slow_times.m_truncate)/1e9,
+        static_cast<float>(m_slow_times.m_unlink)/1e9, static_cast<float>(m_slow_times.m_rename)/1e9, static_cast<float>(m_slow_times.m_chmod)/1e9


Why do all these divisions when you could multiply with 1e-9f?

amadio · 2025-02-07T12:29:26Z

This looks like it's ready for merging. However, if you don't want me to squash commits, please merge fixes into the appropriate commits they are fixing (i.e. the commit with "Address review comments" should just be a fixup of the previous commit, most likely, same for using RAtomic, the code should be added like that in one go).

With this, any OSS plugin may use the gstream monitoring.

The storage statistics OSS keep track of number of server-wide requests and performance, reporting this information through the g-stream. It includes separate counters for "slow" requests, allowing one to get a feel for how many requests go through the server over a configurable threshold (default: 1s).

After refactoring to use the common wrapper base clase, multiple methods in the file, directory, and filesystem for OSS stats were now identical to the default implementation from the wrapper. Remove them to reduce redundant code.

In particular: - Touchup class naming scheme to better match the rest of XRootD - Avoid throwing exceptions on failure; add check to see if the resulting object is valid and either fail or bypass it. - Hide symbol visibility outside the module.

Particularly, - Use `RAtomic` classes where possible. - Cleanup pointer on failure. - Move internal functions into the `XrdOssStats::detail` namespace

amadio · 2025-02-18T10:54:23Z

Updated this to the current devel branch. Will merge it once the CI is green.

amadio · 2025-02-24T10:47:06Z

DEB/RPM generation is not working after merging this into devel. In order to move this to master to release this with 5.8, this needs to be addressed.

amadio · 2025-02-24T11:42:47Z

Please see the errors for RPM and DEB package generation at the respective links.

amadio · 2025-02-24T12:33:19Z

It was easy to fix, so fixed in dcaa75f, 86ef56b and 560e360.

bbockelm force-pushed the stats_oss branch from 1f302ea to 5b721ed Compare May 31, 2024 13:55

amadio requested a review from abh3 May 31, 2024 14:03

bbockelm force-pushed the stats_oss branch from 5b721ed to 7702f18 Compare May 31, 2024 14:26

bbockelm mentioned this pull request Jun 4, 2024

Improved XRootD cache load measurements PelicanPlatform/pelican#1372

Closed

abh3 requested changes Jun 14, 2024

View reviewed changes

amadio force-pushed the devel branch 4 times, most recently from 472b0ee to e358ac0 Compare July 3, 2024 12:40

bbockelm force-pushed the stats_oss branch from 7702f18 to d18a43e Compare July 24, 2024 12:50

amadio force-pushed the devel branch 3 times, most recently from 7e12e96 to b89a1ba Compare July 30, 2024 19:14

amadio force-pushed the devel branch from c5fe44e to 9f9edc8 Compare August 6, 2024 13:55

amadio force-pushed the devel branch from 4fa39e5 to e551c25 Compare August 20, 2024 13:25

bbockelm force-pushed the stats_oss branch from d18a43e to 4361eab Compare August 26, 2024 17:35

bbockelm requested a review from abh3 August 26, 2024 17:54

abh3 requested changes Aug 29, 2024

View reviewed changes

amadio force-pushed the devel branch 2 times, most recently from 48d417e to 9933e04 Compare September 4, 2024 09:56

amadio force-pushed the devel branch from d54785d to f00db34 Compare January 21, 2025 16:01

bbockelm force-pushed the stats_oss branch from 3408b05 to e7c60e5 Compare January 27, 2025 14:11

amadio force-pushed the devel branch 2 times, most recently from 945ec35 to dbf7e44 Compare January 28, 2025 14:32

amadio force-pushed the devel branch from dbf7e44 to 768ff82 Compare February 5, 2025 14:31

amadio force-pushed the stats_oss branch from e7c60e5 to f28e817 Compare February 6, 2025 10:20

abh3 reviewed Feb 6, 2025

View reviewed changes

abh3 approved these changes Feb 6, 2025

View reviewed changes

amadio force-pushed the devel branch from 14432e7 to 653bf57 Compare February 6, 2025 16:11

amadio force-pushed the stats_oss branch from f28e817 to 958e604 Compare February 6, 2025 16:16

amadio reviewed Feb 7, 2025

View reviewed changes

amadio force-pushed the devel branch 2 times, most recently from e9dc92a to c958645 Compare February 14, 2025 12:36

bbockelm added 5 commits February 18, 2025 11:53

Add infrastructure for generic OSS g-stream monitoring

46b2882

With this, any OSS plugin may use the gstream monitoring.

[XrdOssStats] Remove redundant code.

015c315

After refactoring to use the common wrapper base clase, multiple methods in the file, directory, and filesystem for OSS stats were now identical to the default implementation from the wrapper. Remove them to reduce redundant code.

[XrdOssStats] Address comments from code review

5f32251

Particularly, - Use `RAtomic` classes where possible. - Cleanup pointer on failure. - Move internal functions into the `XrdOssStats::detail` namespace

amadio force-pushed the stats_oss branch from 958e604 to 5f32251 Compare February 18, 2025 10:53

amadio merged commit e59d8e9 into xrootd:devel Feb 19, 2025
11 checks passed

amadio added this to the 5.8.0 milestone Feb 19, 2025

osschar mentioned this pull request Feb 28, 2025

[Xrootd] Fix gstream configuration processing. #2442

Merged

Add OSS statistics/monitoring plugin #2279

Add OSS statistics/monitoring plugin #2279

Uh oh!

Conversation

bbockelm commented May 31, 2024

Uh oh!

amadio commented May 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abh3 commented May 31, 2024

Uh oh!

amadio commented Jun 1, 2024

Uh oh!

abh3 left a comment

Choose a reason for hiding this comment

Uh oh!

bbockelm commented Jun 17, 2024

Uh oh!

abh3 commented Jun 17, 2024 via email

Uh oh!

abh3 commented Jun 17, 2024 via email

Uh oh!

bbockelm commented Jul 24, 2024

Uh oh!

xrootd-dev commented Jul 24, 2024 via email

Uh oh!

bbockelm commented Aug 26, 2024

Uh oh!

abh3 commented Aug 26, 2024 via email

Uh oh!

abh3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bbockelm commented Jan 27, 2025

Uh oh!

amadio commented Jan 27, 2025

Uh oh!

bbockelm commented Jan 27, 2025

Uh oh!

abh3 left a comment

Choose a reason for hiding this comment

Uh oh!

abh3 Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

bbockelm Feb 6, 2025

Choose a reason for hiding this comment

Uh oh!

abh3 left a comment

Choose a reason for hiding this comment

Uh oh!

amadio Feb 7, 2025

Choose a reason for hiding this comment

Uh oh!

amadio commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amadio commented Feb 18, 2025

Uh oh!

Uh oh!

amadio commented Feb 24, 2025

Uh oh!

amadio commented Feb 24, 2025

Uh oh!

amadio commented Feb 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amadio commented May 31, 2024 •

edited

Loading

amadio commented Feb 7, 2025 •

edited

Loading