opentelemetry-dotnet-contrib [Instrumentation.StackExchangeRedis] Metrics support

Fixes #1742 Design discussion issue #

Changes

Add the StackExchangeRedis Meter

Merge requirement checklist

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

Jul 30 '24 08:07 tiagodaraujo

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: tiagodaraujo / name: Tiago Araujo (45816c4440e0fea31b056c32baf899bd2d80a9be, 59264cfec63cf06b50ae4ecd1e6d5eea739ea74b, b1662bba05be85f83435ee4eb979eed59c25de6f, d343c6c14798c5224c222f7ea4d042849c716dfd, f3b1d3e869d000c67f2540dae74f20b9d32c87a3, 8d4befa5067be0734b7c910a25a9d0579163213f, eedaf7ceccb2a35ee31eb29bcbb6268c6e82403c, 254211afed9df773cd6f733ca2f77a35dec0eb72)
:white_check_mark: login: Coelho04 / name: Pedro Miguel Almeida Coelho (b0ae03ba6d99d07db47feea851b63d816a00e475, 2fa5346b3e372b1b204f38cba0038be1a090c4c4)

Jul 30 '24 08:07 linux-foundation-easycla[bot]

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.

Jul 30 '24 21:07 matt-hensley

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 :warning:
....StackExchangeRedis/Implementation/RedisMetrics.cs	87.50%	2 Missing :warning:
...s/Implementation/RedisProfilerEntryInstrumenter.cs	97.56%	1 Missing :warning:
...dis/StackExchangeRedisInstrumentationExtensions.cs	97.22%	1 Missing :warning:

Additional details and impacted files

@@            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%)`	:arrow_up:
...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

Jul 30 '24 21:07 codecov[bot]

@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

Jul 31 '24 16:07 tiagodaraujo

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

Aug 01 '24 08:08 Kielek

@tiagodaraujo Thanks for working on this. Can you ensure this PR focuses on one aspect only? If the goal is add metrics support, then leave out all other changes to Activity, as that is making it hard to review. Its totally okay (and encouraged) to send those as separate, smaller PRs, so its easier to focus on this PR with just metrics only