eclipse / microprofile-telemetry

microprofile telemetry
Apache License 2.0
19 stars 18 forks source link

bump OpenTelemetry specification version including refinements #60

Closed JanWesterkamp-iJUG closed 1 year ago

JanWesterkamp-iJUG commented 1 year ago

OpenTelemetry spec version updated to 1.14.0.

Additionally some refinements are done, including the new otel.sdk.disabled=false configuration option and some AsciiDoc refactorings.

JanWesterkamp-iJUG commented 1 year ago

@brunobat and @Emily-Jiang: Can we just merge this first part regarding the spec version and documentation?

It would be the simple way to clean up the merge confict for the other yesterday conflicting changes. I can than rebase to main (with this change) and redo the changes from yesterday, that where done in parallel.

The new one would contain the java version update and test and README updates for the new config property.

If required, I can do a separate one for the NOTICE and REAMDE file refactorings.

JanWesterkamp-iJUG commented 1 year ago

@brunobat now you added the OpenTelemetry Java version change here (instead of https://github.com/eclipse/microprofile-telemetry/pull/64/files), as request by @Emily-Jiang.

Any open concerns about the rest of the changes left here?

Where you would like to get the other necessary changes (TCK, README, NOTICE), here or in https://github.com/eclipse/microprofile-telemetry/pull/64/files?

JanWesterkamp-iJUG commented 1 year ago

Ok, to finish this up, I will do the changes here and delete https://github.com/eclipse/microprofile-telemetry/pull/64/files now.

JanWesterkamp-iJUG commented 1 year ago

@brunobat and @Emily-Jiang, here is the compete PR now, please review.

Emily-Jiang commented 1 year ago

The restyle of spec doc should not happen as the plugin formats the docs. With the changes, when you run mvn verify, all format will be redone. Also the sad news is that Otel Java 1.19 does not align with 1.14 after all. Please see the issue I created here. I think we have to leave without the property name changes.

JanWesterkamp-iJUG commented 1 year ago

The restyle of spec doc should not happen as the plugin formats the docs. With the changes, when you run mvn verify, all format will be redone. Also the sad news is that Otel Java 1.19 does not align with 1.14 after all. Please see the issue I created here. I think we have to leave without the property name changes.

@Emily-Jiang I can run mvn verifiy with my Eclipse IDE (2022-09) and the AsciiDoctor Editor Plugin (2.7.0) without reformatting the document. I.e. one sentence per line is an AsciiDoc best practice... What do you use?

But the other news is really a sad one!

Emily-Jiang commented 1 year ago

The restyle of spec doc should not happen as the plugin formats the docs. With the changes, when you run mvn verify, all format will be redone. Also the sad news is that Otel Java 1.19 does not align with 1.14 after all. Please see the issue I created here. I think we have to leave without the property name changes.

@Emily-Jiang I can run mvn verifiy with my Eclipse IDE (2022-09) and the AsciiDoctor Editor Plugin (2.7.0) without reformatting the document. I.e. one sentence per line is an AsciiDoc best practice... What do you use? I'll check out your PR to verify on vscode, which is the one I am using.

But the other news is really a sad one! I think we can use the property name as defined in 1.14. MP Telemetry Implementations can do the property name translation. In this way, when this pec aligns with OTel 1.14, this spec does not need to change. @brunobat thoughts?

JanWesterkamp-iJUG commented 1 year ago

The restyle of spec doc should not happen as the plugin formats the docs. With the changes, when you run mvn verify, all format will be redone. Also the sad news is that Otel Java 1.19 does not align with 1.14 after all. Please see the issue I created here. I think we have to leave without the property name changes.

@Emily-Jiang I can run mvn verifiy with my Eclipse IDE (2022-09) and the AsciiDoctor Editor Plugin (2.7.0) without reformatting the document. I.e. one sentence per line is an AsciiDoc best practice... What do you use? I'll check out your PR to verify on vscode, which is the one I am using. But the other news is really a sad one! I think we can use the property name as defined in 1.14. MP Telemetry Implementations can do the property name translation. In this way, when this pec aligns with OTel 1.14, this spec does not need to change. @brunobat thoughts?

@Emily-Jiang it looks like the schema 1.14.0 does not contain any changes for 1.13.0: https://opentelemetry.io/schemas/1.14.0

So the only impact might be use of the SPEC_VERSION in git and if the package io.opentelemetry.semconv.resource.attributes contains our SDK property.

If this could not be solved in our implementations temporarily, may be the OTel team could do a patch release (or a minor one) soon? The question is when they might be finished with it. When is their next meeting?

By the way, here we have our SPEC_VERSION to fetch for our spec document automatically so in the future we might only need to configure otel.java.version! ;-) Further investigation might be useful for that direction too.

JanWesterkamp-iJUG commented 1 year ago

@Emily-Jiang I can run mvn verifiy with my Eclipse IDE (2022-09) and the AsciiDoctor Editor Plugin (2.7.0) without reformatting the document. I.e. one sentence per line is an AsciiDoc best practice... What do you use? I'll check out your PR to verify on vscode, which is the one I am using.

On the cli a "mvn clean verify" does not change anything regarding formatting (the log shows 15 files skipped). May be I can add an editorconfig file to prevent VSCode to do weird things, but this must be in sync with the current formatting (i.e. checkstyle etc)...

JanWesterkamp-iJUG commented 1 year ago

@brunobat, @Emily-Jiang please review and merge this, if ok, please.

fmhwong commented 1 year ago

I was told that WithSpan and SpanAttribute have been moved to another package in v1.19.0.

I found that they were marked as deprecated in v1.18.0 which is described here

The package name in the spec is now out of date. Do we have any concern that they have deprecated WithSpan and SpanAttribute and moved to a different package?

We reference WithSpan and SpanAttribute in our spec here

WithSpan v1.18.0 https://github.com/open-telemetry/opentelemetry-java/blob/v1.18.0/extensions/annotations/src/main/java/io/opentelemetry/extension/annotations/WithSpan.java

WithSpan v.19.0 https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/v1.19.0/instrumentation-annotations/src/main/java/io/opentelemetry/instrumentation/annotations/WithSpan.java

brunobat commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: https://github.com/eclipse/microprofile-telemetry/pull/49

fmhwong commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: #49

I see. You made the change in #49 for the TCK but we also need to update the spec.

tevans78 commented 1 year ago

It also appears that the opentelemetry-java-instrumentation/instrumentation-annotations module is still in alpha and is not yet a stable API. https://repo1.maven.org/maven2/io/opentelemetry/instrumentation/opentelemetry-instrumentation-annotations/

Emily-Jiang commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: #49

I see. You made the change in #49 for the TCK but we also need to update the spec.

@JanWesterkamp-iJUG can you also update the package name WithSpan and SpanAttribute here to the package name io.opentelemetry.instrumentation.annotations?

JanWesterkamp-iJUG commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: #49

I see. You made the change in #49 for the TCK but we also need to update the spec.

@JanWesterkamp-iJUG can you also update the package name WithSpan and SpanAttribute here to the package name io.opentelemetry.instrumentation.annotations?

@fmhwong good catch! :-)

@Emily yes, I will do this change for the spec document.

JanWesterkamp-iJUG commented 1 year ago

@fmhwong, @Emily-Jiang , @brunobat I fixed the regression for #49, please review.

Emily-Jiang commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: #49

I see. You made the change in #49 for the TCK but we also need to update the spec.

@JanWesterkamp-iJUG can you also update the package name WithSpan and SpanAttribute here to the package name io.opentelemetry.instrumentation.annotations?

@fmhwong good catch! :-)

@Emily yes, I will do this change for the spec document.

I could not locate the spec update for this one @JanWesterkamp-iJUG ? Did you push the commit?

JanWesterkamp-iJUG commented 1 year ago

@fmhwong this must be a regression this was previously fixed here: #49

I see. You made the change in #49 for the TCK but we also need to update the spec.

@JanWesterkamp-iJUG can you also update the package name WithSpan and SpanAttribute here to the package name io.opentelemetry.instrumentation.annotations?

@fmhwong good catch! :-) @Emily yes, I will do this change for the spec document.

I could not locate the spec update for this one @JanWesterkamp-iJUG ? Did you push the commit?

@Emily it's here: https://github.com/eclipse/microprofile-telemetry/pull/60/commits/2b568b0ab1aa729576f5f9eb3fead8146624e52d

JanWesterkamp-iJUG commented 1 year ago

It also appears that the opentelemetry-java-instrumentation/instrumentation-annotations module is still in alpha and is not yet a stable API. https://repo1.maven.org/maven2/io/opentelemetry/instrumentation/opentelemetry-instrumentation-annotations/

@tevans78: Yes, which means a breaking change can occure at any time - out of our control!

In the particular case we are testing with WithSpan but not SpanAttribute in the TCK and it looks like this part now changed the package name already and hopefully will be stable now - but there is no guarantee.

Therefore I would prefere to decouple MP Telemetry from the MP (6.0) release and make it a standalone component spec besides the MP (Platform), until it is stable, see details here: https://github.com/eclipse/microprofile-telemetry/issues/66

The benefit of that approach would be to be able to do independent releases for MP Telemetry, even major ones, when necessary and do not have to wait for or require to MP doing a major release too.

JanWesterkamp-iJUG commented 1 year ago

@Emily-Jiang here is the requested change: https://github.com/eclipse/microprofile-telemetry/pull/60/commits/c094be0e9711aaede413f5bddc7dee5f06fbae0d

Emily-Jiang commented 1 year ago

It also appears that the opentelemetry-java-instrumentation/instrumentation-annotations module is still in alpha and is not yet a stable API. https://repo1.maven.org/maven2/io/opentelemetry/instrumentation/opentelemetry-instrumentation-annotations/

@tevans78: Yes, which means a breaking change can occure at any time - out of our control!

In the particular case we are testing with WithSpan but not SpanAttribute in the TCK and it looks like this part now changed the package name already and hopefully will be stable now - but there is no guarantee.

Therefore I would prefere to decouple MP Telemetry from the MP (6.0) release and make it a standalone component spec besides the MP (Platform), until it is stable, see details here: #66

The benefit of that approach would be to be able to do independent releases for MP Telemetry, even major ones, when necessary and do not have to wait for or require to MP doing a major release too.

I would not want to delay MP Telemetry for the upstream changes. In MP, we are not afraid of breaking things so that we can move forward faster. If we keep on waiting, new things pops up and changes happen all the time. We might not never be catch the train if we want to wait for the perfect occassion.

Emily-Jiang commented 1 year ago

Thanks @JanWesterkamp-iJUG for your contribution!