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

Move DNS lookup metric outside of .NET semconv #785

Merged
merged 7 commits into from
Mar 7, 2024

Conversation

lmolkova
Copy link
Contributor

@lmolkova lmolkova commented Feb 29, 2024

Fixes #404

Changes

This PR moved DNS lookup duration outside of .NET semantic conventions.

It removes MD generation from .NET metric definition as it's frozen in time and should not get any updates or changes on the common metric.

It helps to untie .NET semconv stability from the unstable OTel parts it depends upon and helps with #781

Merge requirement checklist

@lmolkova lmolkova requested review from a team February 29, 2024 06:39
@lmolkova lmolkova changed the title Move DNS lookup outside of .NET semconv Move DNS lookup metric outside of .NET semconv Feb 29, 2024
@lmolkova
Copy link
Contributor Author

@open-telemetry/dotnet-approvers what do you think about this approach? We have a few more places where .NET either uses experimental attributes on stable metrics or declares common-purpose metrics like http.connection.duration.

The proposal here is to move such things away into general semconv as experimental and, as a precaution, remove table auto-generation from .NET preventing possible breaking changes on .NET docs.

The context: we're explicitly annotating stability on attributes and metrics and realized we have some conflicts. Oops

Copy link
Member

@joaopgrassi joaopgrassi left a comment

Choose a reason for hiding this comment

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

I think this makes sense, given the history behind it (.NET marked it all as stable) and our intention to explicitly mark the stability on all things. Waiting for the .NET folks to chime in.

@vishweshbankwar
Copy link
Member

@open-telemetry/dotnet-approvers what do you think about this approach? We have a few more places where .NET either uses experimental attributes on stable metrics or declares common-purpose metrics like http.connection.duration.

The proposal here is to move such things away into general semconv as experimental and, as a precaution, remove table auto-generation from .NET preventing possible breaking changes on .NET docs.

The context: we're explicitly annotating stability on attributes and metrics and realized we have some conflicts. Oops

@lmolkova The change looks good to me. But I think we should make the note visible on .NET side explaining what does stable mean from .NET's perspective?.

@noahfalk
Copy link
Contributor

noahfalk commented Mar 1, 2024

This seems fine to me. It does raise a larger question... if there was concern about unintended breaking changes on the .NET docs for this metric, does that same risk exist for other metrics?

But I think we should make the note visible on .NET side explaining what does stable mean from .NET's perspective?

I'm not sure we've ever officially defined it, but I interpret it to mean we shipped the behavior and we'd treat it similar to any other .NET API behavior in terms of preserving back-compat. Namely that means we'd try hard not to break it without very good reason and if we did introduce a breaking change it would be officially documented.

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 1, 2024

if there was concern about unintended breaking changes on the .NET docs for this metric, does that same risk exist for other metrics?

@noahfalk not for stable ones, the problem that just became obvious that .NET stable document uses metrics and attributes that are not defined as stable.

I.e. there are no breaking changes coming to http.client.request.duration or any stable attributes, but it is possible that dns.question.name attribute is renamed in the future because from otel standpoint it's experimental.

There are other metrics I want to extract from .NET in follow-up PRs - everything that starts with http.*. Since they are not in the .NET namespace, there is always a chance they will be used by other semconvs and applications.

I'd like to do the same trick I've done in this PR with them.

Assuming we do this, I think we can handle changes in the following way

  1. if OTel stabilizes dns.question.name and corresponding metric without any incompatible changes
    • then .NET semconv can be updated to reference existing metric instead of copy-paste.
  2. if any breaking changes happen on the OTel side, .NET has a choice:
    • to stay with non-standard metric/attribute - then we'll keep copy-paste for .NET
    • deprecate/break in the next major version and follow OTel-defined stable metric/attribute

@joaopgrassi
Copy link
Member

joaopgrassi commented Mar 1, 2024

  • if OTel stabilizes dns.question.name and corresponding metric without any incompatible changes

    • then .NET semconv can be updated to reference existing metric instead of copy-paste.
  • if any breaking changes happen on the OTel side, .NET has a choice:

    • to stay with non-standard metric/attribute - then we'll keep copy-paste for .NET
    • deprecate/break in the next major version and follow OTel-defined stable metric/attribute

This plan makes sense to me. I think it's the best we can do given the circumstances.

@lmolkova
Copy link
Contributor Author

lmolkova commented Mar 4, 2024

@open-telemetry/semconv-dotnet-approver any concerns here?

model/registry/dns.yaml Outdated Show resolved Hide resolved
.chloggen/785.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@trisch-me trisch-me left a comment

Choose a reason for hiding this comment

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

lgtm, left few comments

@jsuereth jsuereth merged commit 88d66b5 into open-telemetry:main Mar 7, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Define general metric for DNS lookup duration
7 participants