-
Notifications
You must be signed in to change notification settings - Fork 3.9k
chore(xcap): adds exporter to summarise capture as a structured log line #20099
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
pkg/xcap/exporter.go
Outdated
| collect.fromRegions("DataObjScan", true, "streamsView.init"). | ||
| filter( | ||
| // object store calls | ||
| "bucket.get", "bucket.getrange", "bucket.attributes", |
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.
observations names are hardcoded, I plan to move the stat definitions to a central place so we can directly refer to them.
3b7987e to
330b472
Compare
330b472 to
a470301
Compare
benclive
left a comment
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.
One question on field naming but otherwise LGTM!
pkg/xcap/summary.go
Outdated
| } | ||
| } | ||
|
|
||
| // Format duration values (keys ending with "duration_ns") |
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.
Do these keys hold nanoseconds specifically? Prometheus-style metrics usually just use "seconds" as the unit so maybe we want to use that, rather than doing something different?
I believe .String() on a duration already formats it into something like "3.23s" so the nanoseconds piece would get ignored anyway
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.
they do hold ns now, i can switch it to secs 👍
| // Close the pipeline to calculate the stats. | ||
| pipeline.Close() | ||
|
|
||
| queueTime, _ := ctx.Value(httpreq.QueryQueueTimeHTTPHeader).(time.Duration) |
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.
Is this queue time accurate? Could we pass it into the ToStatsSummary already and resolve one of the TODOs?
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 doubt it, in the code i see its being set by the old scheduler. I removed it intentionally to track it propery in a follow-up
What this PR does / why we need it:
stats.Resultwhich gets used in query response. Only summary is populated, we can also set dataobj stats but i am not sure if its important as we will using the above summary line for monitoring the new engine.logqlmodel/statsfor the new engineexample:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mddeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR