-
Notifications
You must be signed in to change notification settings - Fork 293
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
[Instrumentation.StackExchangeRedis] Metrics support #1982
[Instrumentation.StackExchangeRedis] Metrics support #1982
Conversation
e8ce71c
to
53891e6
Compare
New database metric semantic conventions are being worked on and should be stabilized soon. It doesn't look what was proposed in open-telemetry/opentelemetry-specification#2070 made it to the spec You can see the current progress in the semantic convention repo: It's been requested to not implement the new metrics until they are marked as stable. That being said, the work here is useful, as converting |
...elemetry.Instrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryInstrumenter.cs
Outdated
Show resolved
Hide resolved
...elemetry.Instrumentation.StackExchangeRedis/Implementation/RedisProfilerEntryInstrumenter.cs
Outdated
Show resolved
Hide resolved
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.
Could you please fix dotnet-format issues?
src/OpenTelemetry.Instrumentation.StackExchangeRedis/CHANGELOG.md
Outdated
Show resolved
Hide resolved
53891e6
to
0c7bbea
Compare
0c7bbea
to
c08e020
Compare
c08e020
to
d161623
Compare
@matt-hensley, like you said, at least the I changed all the attributes, including the tracing ones, paying attention to the 1.26.0 db convention. There is nothing in the convention for the queue and server time, so I suggest A chart to try to show the request, queue and network timeline.
We could have the processing time on the client which would be: client.processing = client.request - client.queue - client.network. But I'll leave this one aside for discussion if you think it's relevant. What do you think about this and what can we do next? Thanks in advance |
d161623
to
0d0201b
Compare
0d0201b
to
03afbec
Compare
03afbec
to
33d9561
Compare
@tiagodaraujo, for future changes, could you please add additional commits to the PR instead of force upgrades? It is a bit easier to follow. While merging we are squashing everything to one commit. |
src/OpenTelemetry.Instrumentation.StackExchangeRedis/Implementation/RedisMetrics.cs
Outdated
Show resolved
Hide resolved
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
…ation/RedisMetrics.cs Co-authored-by: Piotr Kiełkowicz <[email protected]>
166c7ab
to
2fa5346
Compare
This PR was marked stale due to lack of activity. It will be closed in 7 days. |
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Fixes #1742
Design discussion issue #
Changes
Add the StackExchangeRedis Meter
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial changes