[Instrumentation.Http] Support for enrich and filter delegates for http client metric #4075

aunikitin · 2023-01-11T19:10:28Z

Hello, I'd like to have an opportunity to enrich metrics with custom logic (for example uri templates of requested paths). Also I add opportunity to filter some requests because of TODO in MeterProviderBuilderExtensions
I tried to use existing HttpClientInstrumentationOptions but firstly, they have too mutch for metrics and secondly, because in the HttpHandlerMetricsDiagnosticListener we are not allowed to take tags from activity (because of uncontrolled source of filling), that's why delegates in settings don't fit .
Also I wrote some tests and did small refactoring in them (move trace tests in one file; split configuration tests from common, because they don't need running HttpListener).
If my changes are admissible, I will add description to CHANGELOG.md.

Probably it resolves issue

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

Appropriate CHANGELOG.md updated for non-trivial changes
Design discussion issue #
Changes in public API reviewed

linux-foundation-easycla · 2023-01-11T19:10:32Z

The committers listed above are authorized under a signed CLA.

✅ login: aunikitin (43cf01f)

vishweshbankwar

@aunikitin Thanks for your PR. Could you please update the PR to only include changes related to enrich and filter options on metrics?. This will keep it small and easier to review. Any refactoring, renaming can be done separately.

aunikitin · 2023-01-15T08:17:57Z

@vishweshbankwar hello, you want me to rollback changes in tests (for example moving test AddHttpClientInstrumentationUsesOptionsApi to ProviderBuilderExtensionsTests without HttpListener and etc.) or only options renaming?

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

cijothomas

See https://github.com/open-telemetry/opentelemetry-dotnet/pull/4075/files#r1073154671

Not yet ready to make new additions to instrumentation library as we do not have a metrics enrichment concept yet.

cijothomas · 2023-01-21T15:20:09Z

@aunikitin Thanks for the PR. I have triggered CI checks and will review.
There is some pending discussions about potentially making stable releases of Instrumentation Libraries, and we'll need to evaluate all the public APIs at that time, and the Enrich/Filter will be re-evaluated at that time to make sure its in alignment with overall Metrics design. (See #3948 (comment) as well)

src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterOptions.cs

aunikitin · 2023-01-22T09:36:42Z

@cijothomas okay, I'm waiting of your decision.
Can you restart CI? I faced with some issues under Windows, hope they're fixed

Signed-off-by: aunikitin <sanya_nikiti@mail.ru>

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

cijothomas · 2023-01-30T18:06:16Z

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs

-                    {
-                        tags.Add(new KeyValuePair<string, object>(SemanticConventions.AttributeNetPeerPort, request.RequestUri.Port));
-                    }
+            this.EnrichWithHttpRequestMessage(ref tags, request);


same as above...this looks like response message and not request?

In meter, both request and response are used. Should I do the same? I agree that it's more convinient to have response, but it'll conflict with #4098

src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterOptions.cs

src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs

vishweshbankwar · 2023-01-30T19:11:53Z

src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterOptions.cs

+    /// <param name="name">The name of the metric being enriched.</param>
+    /// <param name="requestMessage"><see cref="HttpRequestMessage"/>: the HttpRequestMessage object.</param>
+    /// <param name="tags"><see cref="TagList"/>: List of current tags. You can add additional tags to this list. </param>
+    public delegate void HttpRequestMessageEnrichmentFunc(string name, HttpRequestMessage requestMessage, ref TagList tags);


I think we need to pass in both request and response.
#4098
@cijothomas - FYI

I'll leave HttpResponse, if your changes appear earlier, I'll fix mine, and vice versa

…lemetry#4072)

…etry#4092) * Remove the dependency on Microsoft.Extensions.Configuration.EnvironmentVariables from SDK. * Added notes. * CHANGELOG patch.

Signed-off-by: Alex <sanya_nikiti@mail.ru>

# Conflicts: # src/OpenTelemetry/CHANGELOG.md # src/OpenTelemetry/OpenTelemetry.csproj

Signed-off-by: Alex <sanya_nikiti@mail.ru>

github-actions · 2023-02-20T03:18:10Z

This PR was marked stale due to lack of activity and will be closed in 7 days. Commenting or Pushing will instruct the bot to automatically remove the label. This bot runs once per day.

github-actions · 2023-02-27T03:18:19Z

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

aunikitin requested a review from a team January 11, 2023 19:10

aunikitin force-pushed the enrich_filter_in_http_client_metrics branch 3 times, most recently from 23b526a to 43cf01f Compare January 12, 2023 07:04

aunikitin mentioned this pull request Jan 12, 2023

Enrich support for HttpClientMetrics open-telemetry/opentelemetry-dotnet-contrib#1775

Open

vishweshbankwar suggested changes Jan 13, 2023

View reviewed changes

aunikitin force-pushed the enrich_filter_in_http_client_metrics branch 2 times, most recently from 4dbf5b9 to dcb5abf Compare January 15, 2023 08:23

aunikitin changed the title ~~Support for enrich and filter delegates for http client metric~~ [Instrumentation.Http] Support for enrich and filter delegates for http client metric Jan 17, 2023

cijothomas reviewed Jan 18, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs Outdated Show resolved Hide resolved

cijothomas requested changes Jan 18, 2023

View reviewed changes

aunikitin requested review from cijothomas and vishweshbankwar and removed request for cijothomas and vishweshbankwar January 19, 2023 16:34

aunikitin force-pushed the enrich_filter_in_http_client_metrics branch from 2b54dd5 to 75b8411 Compare January 21, 2023 15:02

cijothomas reviewed Jan 21, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterOptions.cs Outdated Show resolved Hide resolved

aunikitin added 4 commits January 23, 2023 10:53

Support for enrich and filter delegates for http client metric

e62cb93

Rollback options renaming

6db0e5e

Signed-off-by: aunikitin <sanya_nikiti@mail.ru>

Remove allocations and update public contract

de6b35d

Signed-off-by: aunikitin <sanya_nikiti@mail.ru>

one more fix for public contract

4b587cf

Signed-off-by: aunikitin <sanya_nikiti@mail.ru>

aunikitin force-pushed the enrich_filter_in_http_client_metrics branch from d5bdb44 to 4b587cf Compare January 23, 2023 07:56

fix windows building issues

7bf195f

Signed-off-by: aunikitin <sanya_nikiti@mail.ru>

aunikitin requested a review from cijothomas January 30, 2023 09:32

cijothomas reviewed Jan 30, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/Implementation/HttpHandlerMetricsDiagnosticListener.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jan 30, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/HttpClientInstrumentationMeterOptions.cs Outdated Show resolved Hide resolved

cijothomas reviewed Jan 30, 2023

View reviewed changes

src/OpenTelemetry.Instrumentation.Http/MeterProviderBuilderExtensions.cs Outdated Show resolved Hide resolved

vishweshbankwar reviewed Jan 30, 2023

View reviewed changes

CodeBlanch and others added 8 commits February 4, 2023 19:17

[SDK] Add a test covering exception thrown in custom sampler (open-te…

df9125f

…lemetry#4072)

[SDK] Remove the dependency on M.E.C.EnvironmentVariables (open-telem…

c7d9f09

…etry#4092) * Remove the dependency on Microsoft.Extensions.Configuration.EnvironmentVariables from SDK. * Added notes. * CHANGELOG patch.

cleanup tests and docs (open-telemetry#4099)

ce56a77

[SDK] Remove the dependency on M.E.Options 5.0 (open-telemetry#4093)

dbef1d5

draft (open-telemetry#4106)

88c9a22

Encourage best practices in the logging tutorial (open-telemetry#4094)

1064705

Minor refactor on histogram metric update (open-telemetry#4111)

6689dca

Replace Request with Response, hide meter tests from net462

ebb41ce

Signed-off-by: Alex <sanya_nikiti@mail.ru>

aunikitin force-pushed the enrich_filter_in_http_client_metrics branch 2 times, most recently from ff8550b to ebb41ce Compare February 4, 2023 16:37

aunikitin added 2 commits February 4, 2023 19:54

Merge branch 'main' into enrich_filter_in_http_client_metrics

f9d944a

# Conflicts: # src/OpenTelemetry/CHANGELOG.md # src/OpenTelemetry/OpenTelemetry.csproj

replace ConfigureBuilder with old api

8180379

Signed-off-by: Alex <sanya_nikiti@mail.ru>

aunikitin requested a review from cijothomas February 4, 2023 16:57

aunikitin marked this pull request as draft February 8, 2023 10:58

aunikitin marked this pull request as ready for review February 12, 2023 21:07

github-actions bot added the Stale Issues and pull requests which have been flagged for closing due to inactivity label Feb 20, 2023

github-actions bot closed this Feb 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Instrumentation.Http] Support for enrich and filter delegates for http client metric #4075

[Instrumentation.Http] Support for enrich and filter delegates for http client metric #4075

aunikitin commented Jan 11, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 11, 2023 •

edited

Loading

vishweshbankwar left a comment •

edited

Loading

aunikitin commented Jan 15, 2023 •

edited

Loading

cijothomas left a comment

cijothomas commented Jan 21, 2023

aunikitin commented Jan 22, 2023

cijothomas Jan 30, 2023

aunikitin Feb 4, 2023

vishweshbankwar Jan 30, 2023

aunikitin Feb 4, 2023

github-actions bot commented Feb 20, 2023

github-actions bot commented Feb 27, 2023

[Instrumentation.Http] Support for enrich and filter delegates for http client metric #4075

[Instrumentation.Http] Support for enrich and filter delegates for http client metric #4075

Conversation

aunikitin commented Jan 11, 2023 • edited Loading

Changes

linux-foundation-easycla bot commented Jan 11, 2023 • edited Loading

vishweshbankwar left a comment • edited Loading

Choose a reason for hiding this comment

aunikitin commented Jan 15, 2023 • edited Loading

cijothomas left a comment

Choose a reason for hiding this comment

cijothomas commented Jan 21, 2023

aunikitin commented Jan 22, 2023

cijothomas Jan 30, 2023

Choose a reason for hiding this comment

aunikitin Feb 4, 2023

Choose a reason for hiding this comment

vishweshbankwar Jan 30, 2023

Choose a reason for hiding this comment

aunikitin Feb 4, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 20, 2023

github-actions bot commented Feb 27, 2023

aunikitin commented Jan 11, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jan 11, 2023 •

edited

Loading

vishweshbankwar left a comment •

edited

Loading

aunikitin commented Jan 15, 2023 •

edited

Loading