Skip to content
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

Closed

Conversation

tiagodaraujo
Copy link

@tiagodaraujo tiagodaraujo commented Jul 30, 2024

Fixes #1742
Design discussion issue #

Changes

Add the StackExchangeRedis Meter

Merge requirement checklist

  • CONTRIBUTING guidelines followed (license requirements, nullable enabled, static analysis, etc.)
  • Unit tests added/updated
  • Appropriate CHANGELOG.md files updated for non-trivial changes
  • Changes in public API reviewed (if applicable)

Copy link

linux-foundation-easycla bot commented Jul 30, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@matt-hensley
Copy link
Contributor

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:
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/database/database-metrics.md

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 redis.client.request.duration to the new db.client.operation.duration looks straightforward.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 94.16667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.83%. Comparing base (71655ce) to head (067ae78).
Report is 453 commits behind head on main.

Files with missing lines Patch % Lines
...tackExchangeRedisMeterProviderBuilderExtensions.cs 72.72% 3 Missing ⚠️
....StackExchangeRedis/Implementation/RedisMetrics.cs 87.50% 2 Missing ⚠️
...s/Implementation/RedisProfilerEntryInstrumenter.cs 97.56% 1 Missing ⚠️
...dis/StackExchangeRedisInstrumentationExtensions.cs 97.22% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1982      +/-   ##
==========================================
- Coverage   73.91%   70.83%   -3.09%     
==========================================
  Files         267      338      +71     
  Lines        9615    12587    +2972     
==========================================
+ Hits         7107     8916    +1809     
- Misses       2508     3671    +1163     
Flag Coverage Δ
unittests-Exporter.Geneva 53.32% <ø> (?)
unittests-Exporter.InfluxDB 95.88% <ø> (?)
unittests-Exporter.Instana 71.24% <ø> (?)
unittests-Exporter.OneCollector 94.32% <ø> (?)
unittests-Exporter.Stackdriver 75.73% <ø> (?)
unittests-Extensions 88.57% <ø> (?)
unittests-Extensions.AWS 83.41% <ø> (?)
unittests-Extensions.Enrichment 100.00% <ø> (?)
unittests-Instrumentation.AWS 84.78% <ø> (?)
unittests-Instrumentation.AWSLambda 88.92% <ø> (?)
unittests-Instrumentation.AspNet 76.73% <ø> (?)
unittests-Instrumentation.AspNetCore 85.27% <ø> (?)
unittests-Instrumentation.ConfluentKafka 13.32% <ø> (?)
unittests-Instrumentation.ElasticsearchClient 79.87% <ø> (?)
unittests-Instrumentation.EntityFrameworkCore 55.49% <ø> (?)
unittests-Instrumentation.EventCounters 76.36% <ø> (?)
unittests-Instrumentation.GrpcNetClient 79.61% <ø> (?)
unittests-Instrumentation.Hangfire 93.58% <ø> (?)
unittests-Instrumentation.Http 82.05% <ø> (?)
unittests-Instrumentation.Owin 85.97% <ø> (?)
unittests-Instrumentation.Process 100.00% <ø> (?)
unittests-Instrumentation.Quartz 78.94% <ø> (?)
unittests-Instrumentation.Runtime 100.00% <ø> (?)
unittests-Instrumentation.SqlClient 91.89% <ø> (?)
unittests-Instrumentation.StackExchangeRedis 72.18% <94.16%> (?)
unittests-Instrumentation.Wcf 78.47% <ø> (?)
unittests-PersistentStorage 65.78% <ø> (?)
unittests-Resources.AWS 77.93% <ø> (?)
unittests-Resources.Azure 82.35% <ø> (?)
unittests-Resources.Container 72.41% <ø> (?)
unittests-Resources.Gcp 72.54% <ø> (?)
unittests-Resources.Host 72.64% <ø> (?)
unittests-Resources.OperatingSystem 77.20% <ø> (?)
unittests-Resources.Process 100.00% <ø> (?)
unittests-Resources.ProcessRuntime 94.11% <ø> (?)
unittests-Sampler.AWS 87.61% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...dis/StackExchangeRedisConnectionInstrumentation.cs 98.57% <100.00%> (+1.51%) ⬆️
...ExchangeRedis/StackExchangeRedisInstrumentation.cs 100.00% <100.00%> (ø)
...ackExchangeRedisTracerProviderBuilderExtensions.cs 66.66% <100.00%> (ø)
...s/Implementation/RedisProfilerEntryInstrumenter.cs 43.97% <97.56%> (ø)
...dis/StackExchangeRedisInstrumentationExtensions.cs 97.22% <97.22%> (ø)
....StackExchangeRedis/Implementation/RedisMetrics.cs 87.50% <87.50%> (ø)
...tackExchangeRedisMeterProviderBuilderExtensions.cs 72.72% <72.72%> (ø)

... and 357 files with indirect coverage changes

Copy link
Contributor

@Kielek Kielek left a 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?

@tiagodaraujo
Copy link
Author

tiagodaraujo commented Jul 31, 2024

@matt-hensley, like you said, at least the redis.client.request.duration follows the convention.

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 redis.client.queue.duration and redis.client.network.duration. They may be useful in understanding redis.client.request.duration better as it can fluctuate depending on the queue time or the waiting time for the server.

A chart to try to show the request, queue and network timeline.

client.request |---------------------------|
client.queue     |----|
client.network        |-------------------|

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

@Kielek
Copy link
Contributor

Kielek commented Aug 1, 2024

@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.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Oct 31, 2024
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Nov 15, 2024
Copy link
Contributor

Closed as inactive. Feel free to reopen if this PR is still being worked on.

@github-actions github-actions bot closed this Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:instrumentation.stackexchangeredis Things related to OpenTelemetry.Instrumentation.StackExchangeRedis Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Metric instrumentation for Redis client
6 participants