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

Add the plan review for MicroProfile Telemetry 1.0 #168

Merged
merged 3 commits into from
Sep 6, 2022

Conversation

Emily-Jiang
Copy link
Member

No description provided.

Copy link

@brunobat brunobat left a comment

Choose a reason for hiding this comment

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

Looks good to me @Emily-Jiang

** Automatic Instrumention: Jakarta RESTful Web Services (server and client), and MicroProfile REST Clients are automatically enlisted to participate in distributed tracing without code modification as specified in the Tracing API.
** Manual Instrumentation: explicit manual instrumentation can be added into a MicroProfile application using annotation or programmatic lookup.
** Agent Instrumentation: implementations are free to support the OpenTelemetry Agent Instrumentation.
* OpenTelemetry Tracing is deliberately set off unless end users explicitly enable it via the configuration defined in this specification.
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I provide a suggestion to clarify this sentence further, because I think people that weren't around in the discussions regarding this specific topic might miss some information?

"OpenTelemetry Tracing is deliberately disabled unless end users explicitly enable it via the configuration defined in this specification. This means that when OpenTelemetry Tracing is disabled, MicroProfile OpenTracing will still be the platform's activated default"

Copy link

Choose a reason for hiding this comment

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

+1 to @ederks85 .

Copy link

@brunobat brunobat Aug 16, 2022

Choose a reason for hiding this comment

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

I agree with the clarification but I would probably add at the end something:

  • OpenTelemetry Tracing is deliberately disabled unless end users explicitly enable it via the configuration defined in this specification. This means that when OpenTelemetry Tracing is disabled, MicroProfile OpenTracing will still be the platform's activated default
  • MicroProfile OpenTracing will be deprecated and the switch to Microprofile Telemetry is recommended.

Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand the current plan for MP 6.0 is the following:

  • remove MP OpenTracing from the MicroProfile umbrella spec, but keep it as a separate (optional) MicroProfile component specification
  • add MP Telemetry Tracing to the MicroProfile umbrella spec, but have it deactivated by default - to use it, it must be activated manually

So by default, there is no Tracing technology active in MP 6.0.
This behaviour should include the propagation of traces - this might be a separate config in OTel itself (there it is acticve by default) and in the context of MicroProfile it should be set according to the OTel SDK, where the decision was made to have default off behaviour. Having propagation actived by default, but expecting MP Telemetry Tracing to be off is not expected by users and could open security attack vectors.
This is an issue inside MP Telemetry Tracing and not Otel itself (there it is both on by default), because it is added to the umbrella spec by default, but expected to be off by default.

In general, it is a good idea to not have multiple tracing technologies active by default to prevent wasting resources. But there are valid use cases where this sould be possible, i.e. when in a transition phase from OpenTracing to OpenTelemetry Tracing of a big micro service landscape - this could ususally take time and could not be done in a big bang scenario. During the transition, you do not want to lose insights on your service calls, so you need to add OpenTelemtry Tracing additionally to all services, before you remove OpenTracing support of them and stop using the corresponding old backend.

Copy link
Member Author

@Emily-Jiang Emily-Jiang Aug 30, 2022

Choose a reason for hiding this comment

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

In this plan review, I will not add MP OpenTracing in the mix. I would rephrase:
MicroProfile Telemetry Tracing is deliberately disabled unless end users explicitly enable it via the configuration defined in this specification. This means end users will not get surprises by many tracing generated unless instructed."

Copy link
Member Author

@Emily-Jiang Emily-Jiang Aug 30, 2022

Choose a reason for hiding this comment

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

MP OpenTracing will not be part of MP 6.0. It will become standalone. The relationship between MP Telemetry and MP OpenTracing should be clearly specified in MP umbrella spec (see issue here).

Copy link
Contributor

Choose a reason for hiding this comment

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

In this plan review, I will not add MP OpenTracing in the mix. I would rephrase: MicroProfile Telemetry Tracing is deliberately disabled unless end users explicitly enable it via the configuration defined in this specification. This means end users will not get surprises by many tracing generated unless instructed."

@Emily-Jiang +1 for your wording

@Emily-Jiang
Copy link
Member Author

@ederks85 are you happy with the explanation above? If you do, please approve this PR. @brunobat and @JanWesterkamp-iJUG have agreed with the rewording.

@ederks85
Copy link
Collaborator

ederks85 commented Sep 6, 2022

This looks fine now, especially this is very important to have in order to understand that we are adopting a specification:

"OpenTelemetry APIs/SPIs are adopted by this specification and no additional API/SPIs are defined in this specification."

@Emily-Jiang Emily-Jiang merged commit a45e1e7 into microprofile:main Sep 6, 2022
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.

5 participants