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

use seconds for OTLP export #3875

Closed
wants to merge 4 commits into from

Conversation

zeitlinger
Copy link
Contributor

use seconds for otlp export, as specified in open-telemetry/opentelemetry-java-instrumentation#8355

@pivotal-cla
Copy link

@zeitlinger Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-cla
Copy link

@zeitlinger Thank you for signing the Contributor License Agreement!

@jonatan-ivanov
Copy link
Member

jonatan-ivanov commented Jun 6, 2023

Thanks for the PR!
Your link points to an issue in the Micrometer Bridge. I guess you meant this as "specified": open-telemetry/opentelemetry-specification#3312

Based on the PR, the semantic conventions spec says that:

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

I found this somewhat confusing. Why does the semantic conventions spec says anything about how to measure time, shouldn't it be in the SDK specs? Can one convention define one unit and another one define a different one so you get into a conflict? Why is it specified at all since OTel does not have a Timer concept.

Based on this, I don't think we should merge this PR since:

  • Micrometer always measures duration in nanos and then converts it to the unit of the registry. No OTel specification should have any affect on "measuring durations" in Micrometer (this PR does not change that).
  • Users of Micrometer may or may not use the OTel semantic conventions (please note that semantic conventions spec should not have any effect on OTLP) and OTLP does not define a time unit
  • This is a breaking change

What do you think?

@jonatan-ivanov jonatan-ivanov added the waiting for feedback We need additional information before we can continue label Jun 6, 2023
@lenin-jaganathan
Copy link
Contributor

@jonatan-ivanov Do you think it still makes sense to address the referenced issue? I mean providing configurable "baseUnit"?

@jonatan-ivanov
Copy link
Member

@lenin-jaganathan Definitely, especially because the OTel Collector is not converting time units to the backend that is forwarding to so the component that emits OTLP have to. This is very unfortunate but it's not a huge change on Micrometer's side.

@zeitlinger
Copy link
Contributor Author

@lenin-jaganathan Definitely, especially because the OTel Collector is not converting time units to the backend that is forwarding to so the component that emits OTLP have to. This is very unfortunate but it's not a huge change on Micrometer's side.

I've given it a try: #3883

@zeitlinger
Copy link
Contributor Author

  • Users of Micrometer may or may not use the OTel semantic conventions

This is a key question - how does micrometer fit into OpenTelemetry ecosystem?

OTLP is a building block, but the otel semantic conventions are an important building block as well - becoming more important as they are stabilizing.

One use case is an otel JVM dashboard. Would micrometer aim to support this once the semantic conventions are stable?

@jonatan-ivanov
Copy link
Member

Yes, we are planning to look into providing support for OTel Semconv (we also contributed to it).

@jonatan-ivanov
Copy link
Member

Superseded by #3883

@jonatan-ivanov jonatan-ivanov added superseded An issue that has been superseded by another and removed waiting for feedback We need additional information before we can continue labels Jun 10, 2023
@lenin-jaganathan
Copy link
Contributor

@jonatan-ivanov Just wanted to get more clarity on your comment

we are planning to look into providing support for OTel Semconv.

Does this mean JVM Metrics generated by micrometer will adhere to OTEL Sem conv for JVM Metrics?
I mean some MeterFilter similar to something here to convert all the JVM metrics to OTEL semconv equivalent?

And are there plans on adding filters for other binders as well. E.g: OTEL has sem conv for HTTP metrics and how is micrometer planning to adapt it?

@jonatan-ivanov
Copy link
Member

Does this mean JVM Metrics generated by micrometer will adhere to OTEL Sem conv for JVM Metrics?
I mean some MeterFilter similar to something here to convert all the JVM metrics to OTEL semconv equivalent?

We can't promise it will since right now semconv is not stable but once it becomes stable and gains popularity we are planning to look into providing that feature to the users. This can be implemented via Micrometer providing MeterFilters or adding variants/flags to the current instrumentation to produce data that is compliant to semconv. We must keep current behavior too (to not introduce breaking chnges).

And are there plans on adding filters for other binders as well. E.g: OTEL has semconv for HTTP metrics and how is micrometer planning to adapt it?

I think the above should be applicable everywhere it makes sense (http, mongo, etc). I think the "how" will be a question for every single instrumentation.

@zeitlinger zeitlinger deleted the otlp_base_unit branch June 12, 2023 09:47
@zeitlinger
Copy link
Contributor Author

Yes, we are planning to look into providing support for OTel Semconv (we also contributed to it).

that's great to hear.

I think it would make sense to come back to this topic (making seconds the default base unit) together with the otel semconv then - because both topics will help users to re-use dashboards for all of their java applications.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
superseded An issue that has been superseded by another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants