Skip to content

feat(telemetry): add telemetry support for user agent parsing#90

Merged
BenjaminAbt merged 20 commits into
mainfrom
feature/add-fluentapi-and-telemetry
Feb 19, 2026
Merged

feat(telemetry): add telemetry support for user agent parsing#90
BenjaminAbt merged 20 commits into
mainfrom
feature/add-fluentapi-and-telemetry

Conversation

@BenjaminAbt

@BenjaminAbt BenjaminAbt commented Dec 27, 2025

Copy link
Copy Markdown
Member

@BenjaminAbt BenjaminAbt requested a review from gfoidl December 27, 2025 15:17

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the event source, I'd prefer to use OTel metrics via the meters.
Especially as we don't write to the event stream (which could be read e.g. via PerfView), and I don't think we need the diagnostic events.

internal void UserAgentPresent()
{
if (!IsEnabled()) return;
_userAgentPresent?.Increment();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we don't write an event, should we use metrics (via meter factory) instead?
I'd prefer the OTel metrics here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are too many projects that do not use OTEL - even I/we currently still use native app insights in almost all projects because OTEL features are missing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, with UseAzureMonitor I see metrics in Azure. What features are there missing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we would still enforce everybody to use OTEL as foundation... I am not happy with that.

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then OK.

@BenjaminAbt BenjaminAbt requested a review from gfoidl December 27, 2025 19:37
@BenjaminAbt

BenjaminAbt commented Dec 27, 2025

Copy link
Copy Markdown
Member Author

@gfoidl , I added a telemetry hub and exposed otel native meters and dotnet metrics.
I think I dont have added any overhead.

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the OTel attributes we should follow the semantic convention (now, because changing it later would be a breaking change).

Sorry for the delay.

@BenjaminAbt

Copy link
Copy Markdown
Member Author

@gfoidl no problem. I expected the delay, since it was the holidays. Happy New Year.

@BenjaminAbt

Copy link
Copy Markdown
Member Author

@gfoidl there is this note

When instruments are measuring durations, seconds (i.e. s) SHOULD be used.

does this really make sense? We have parse.duration which is always 0s because its way faster than 0s

@gfoidl

gfoidl commented Feb 2, 2026

Copy link
Copy Markdown
Contributor

does this really make sense? We have parse.duration which is always 0s because its way faster than 0s

Makes sense, but the value has to be given as floating point number. Then the tools to display should show it as ms or that like.

@BenjaminAbt BenjaminAbt requested a review from gfoidl February 5, 2026 11:58
Comment thread src/HttpUserAgentParser.AspNetCore/readme.md Outdated
Comment thread src/HttpUserAgentParser.MemoryCache/readme.md Outdated
Comment thread src/HttpUserAgentParser/Telemetry/HttpUserAgentParserEventSource.cs Outdated
Comment thread src/HttpUserAgentParser/readme.md
Comment thread src/HttpUserAgentParser/readme.md Outdated
Comment thread src/HttpUserAgentParser/readme.md Outdated
Comment thread README.md
@BenjaminAbt

BenjaminAbt commented Feb 6, 2026

Copy link
Copy Markdown
Member Author

@gfoidl

the event source is

        _concurrentCacheMiss = new IncrementingEventCounter("cache-miss", this)
        {
            DisplayName = "Cache miss",
            DisplayUnits = "calls",
        };

the meter is

        s_concurrentCacheMiss = s_meter.CreateCounter<long>(
            name: "cache.miss",
            unit: "{call}",
            description: "Cache miss");

so calls vs call ..

Should that match as best practise?

@gfoidl

gfoidl commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

Should that match as best practise?

Good question.

For the meter the singular "call" is correct, as the display end will use the pluralized form when needed.
For the event source, respectively the display ends no such pluralization is done automatically or at least there's no written down convention that dictates to do so. As "calls" is correct for the event source, I think it's best to keep them as they are now (so "calls" for the event source, and "call" for the meter).

@gfoidl gfoidl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the shared code, otherwise LGTM.

Comment thread src/HttpUserAgentParser/Telemetry/HttpUserAgentParserMeters.cs
@BenjaminAbt BenjaminAbt merged commit aa9ad4f into main Feb 19, 2026
3 checks passed
@BenjaminAbt BenjaminAbt deleted the feature/add-fluentapi-and-telemetry branch February 19, 2026 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add telemetry (aka event-counters)

2 participants