-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Adds instrumentation to PUT ops in the CLI #18139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Adds instrumentation around put_opts - Adds instrumentation around put_multipart - Adds tests for newly instrumented methods
op: Operation::Put, | ||
path: location.clone(), | ||
timestamp, | ||
duration: Some(elapsed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm feeling a bit torn on using a duration here. Unlike list()
this duration is accurate for what's happening, however, I fear it may be misleading. The duration for a multipart put will just be the duration spent initiating a multipart put session with the backing store. It won't be able to capture the true duration of uploading any data, which is what I think a user would expect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we log a ticket to try and make the accounting more accurate.
In general I think trying to get the level of timing might be a better case for https://github.com/datafusion-contrib/datafusion-tracing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's probably a good way to address this. I noted that you made one for tracking duration for list
and this probably falls into a similar category.
Prior to closing the overarching issue for these PRs should document some of the current caveats for duration metrics?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prior to closing the overarching issue for these PRs should document some of the current caveats for duration metrics?
That sounds good to me -- I suggest putting it in the code (not just the docs) so it is more discoverable -- maybe a note after the summary output?
Object Store Profiling
....
Put
count: 1
duration min: 0.000249s
duration max: 0.000249s
duration avg: 0.000249s
size min: 815 B
size max: 815 B
size avg: 815 B
size sum: 815 B
*** NEW ***
Note: Duration for multipart PUT is time spent initiating a multipart PUT session with the backing store. It does not include the time to actually upload data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me -- thank you @BlakeOrth
I merged up from main to resolve a conflict |
Which issue does this PR close?
This does not fully close, but is an incremental building block component for:
datafusion-cli
] Add a way to see what object store requests are made #17207The full context of how this code is likely to progress can be seen in the POC for this effort:
Rationale for this change
Further fills out the missing methods that have yet to be instrumented in the instrumented object store.
What changes are included in this PR?
Are these changes tested?
Yes. Unit tests have been added for the new methods
Example output:
(note: I have no idea how to exercise/show a multi-part put operation, or if DataFusion even utilizes multipart puts for large files)
Are there any user-facing changes?
No-ish
cc @alamb