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 OpenTelemetry OTLP Exporter #17641

Merged
merged 3 commits into from
Jun 8, 2021
Merged

Add OpenTelemetry OTLP Exporter #17641

merged 3 commits into from
Jun 8, 2021

Conversation

luneo7
Copy link
Contributor

@luneo7 luneo7 commented Jun 2, 2021

Adds OpenTelemetry OTLP Exporter and addresses features requests in #17640

@quarkus-bot quarkus-bot bot added the area/core label Jun 2, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 2, 2021

/cc @kenfinnigan

@quarkus-bot quarkus-bot bot added area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/tracing labels Jun 2, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Jun 2, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building d736006

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 2, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 68f046d

Status Name Step Test failures Logs Raw logs
Initial JDK 11 Build Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 3, 2021

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 7c1451c

Status Name Step Test failures Logs Raw logs
Native Tests - Misc4 Build ⚠️ Check → Logs Raw logs

@kenfinnigan
Copy link
Member

@luneo7 thanks for the contribution!

I have a few suggestions on the current changes:

  1. Don't worry about moving the LateBoundBatchSpanProcessor to a common extension, just duplicate it within the two exporter extensions. The reason is I intend to propose the class to the OpenTelemetry Java project soon, so there should be a point where we don't need it at all in Quarkus code and we can use it from OpenTelemetry.
  2. It would be preferred to separate the changes for adding an OTLP Exporter and modifying how the providers are configured into separate PRs to keep the changes isolated from one another. This helps with any potential backporting of a change to an older release
  3. We need a slightly different approach for Propagators and Resources as there can be many of them as opposed to one. It was on my list to look at soon, so you can continue to work on a solution with my guidance, or I can look to add it, whatever you prefer.
  4. In the IdGenerator case, if we were able to dispense with the @DefaultBean and only set it onto the provider builder if one was present that would be preferable as it saves on creating a duplicate object instance.

Let me know if you have any questions.

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 3, 2021

Hi @kenfinnigan =]

I can do 1, 2, and 4 right away no problem =]

About 3, I thought of having a producer of ContextPropagators because this would contain all propagators, so someone can override the bean and produce something like ContextPropagators.create(TextMapPropagator.composite(W3CTraceContextPropagator.getInstance(), W3CBaggagePropagator.getInstance())); . But we could also just do a bean lookup of TextMapPropagator and in the recorder join then in a TextMapPropagator.composite that would do mostly the same thing in the end.

About the Resource, I kind thought about the same way. If someone wants multiple resources they can use .merge within the bean producer, or we can have multiple beans and let the recorder join them, the only thing would be the order of precedence, since when merging a desired property can be left out since it they are mostly a List and before adding it checks if the key is already present.

What do you think? Do you already have some proposal for those?

Happy to help =]

@kenfinnigan
Copy link
Member

Thanks @luneo7, if you could do 1, 2, 4, that would be greatly appreciated!

As for 3, I hadn't done anything beyond some thinking of the various options.

For propagators, I was thinking of providing a config option to define the propagators you want to be installed, similar to what OpenTelemetry Java AutoConfigure does, but doing so in a way that doesn't require the extension to depend on the artifact which includes them. So it would need some mapping of "type name" to "class name string" so we can see if the class exists before we install a single or composite propagator.

Though we could have user applications create composite propagators, if we can keep it simple to do the developer experience should be improved. Do you agree?

Resources are certainly more tricky, but there are also some aspects of the AutoConfigure extension providing default resources for environment, process, etc which could be interesting to provide without a developer having to construct a Resource with all the necessary bits

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 3, 2021

One thing about AutoConfigure is that they created one SPI, and they are using compileOnly dependencies so they can use some classes there without doing reflection. If we were going to do a class mapping, we would need more than the class name, because some of them have different methods to get the instance, and we would have to do reflection to instantiate the propagators, is that OK (like B3Propagator.injectingSingleHeader() and B3Propagator.injectingMultiHeaders()?

@kenfinnigan
Copy link
Member

Yeah, I saw how they did that, and I'd certainly like to avoid the need for reflection.

I've asked a question on whether there's a reason for some differences between the propagators, as the AWS propagator implements a provider with an SPI method for retrieving the instance.

If the other propagators had providers as well it would make implementation easier.

Otherwise, I don't have a good answer for how to solve it right now.

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 4, 2021

@kenfinnigan Updated the PR, now it has only the OTLP exporter

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 4, 2021

@kenfinnigan Created another PR to address the config params: #17703

Copy link
Member

@kenfinnigan kenfinnigan left a comment

Choose a reason for hiding this comment

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

Thanks @luneo7, looks really good.

Had a couple of minor comments, but the big question is whether we should have a section in the OpenTelemetry Guide talking about how to use the OTLP Exporter instead of the Jaeger one. What do you think?

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 4, 2021

This workflow status is outdated as a new workflow run has been triggered.

🚫 This workflow run has been cancelled.

Failing Jobs - Building fe49742

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
Gradle Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
JVM Tests - JDK 16 Build Test failures Logs Raw logs
Maven Tests - JDK 11 Build ⚠️ Check → Logs Raw logs
Maven Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Verify ⚠️ Check → Logs Raw logs
Native Tests - Amazon Build ⚠️ Check → Logs Raw logs
Native Tests - Cache ⚠️ Check → Logs Raw logs
Native Tests - Data1 Build ⚠️ Check → Logs Raw logs
Native Tests - Data2 Build ⚠️ Check → Logs Raw logs
Native Tests - Data3 Build ⚠️ Check → Logs Raw logs
Native Tests - Data4 Build ⚠️ Check → Logs Raw logs
Native Tests - Data6 Build ⚠️ Check → Logs Raw logs
Native Tests - Data7 Build ⚠️ Check → Logs Raw logs
Native Tests - HTTP ⚠️ Check → Logs Raw logs
Native Tests - Messaging1 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Messaging2 ⚠️ Check → Logs Raw logs
Native Tests - Misc1 ⚠️ Check → Logs Raw logs
Native Tests - Misc2 ⚠️ Check → Logs Raw logs
Native Tests - Misc3 ⚠️ Check → Logs Raw logs
Native Tests - Misc4 ⚠️ Check → Logs Raw logs
Native Tests - Security1 ⚠️ Check → Logs Raw logs
Native Tests - Security2 ⚠️ Check → Logs Raw logs
Native Tests - Security3 ⚠️ Check → Logs Raw logs
Native Tests - Spring ⚠️ Check → Logs Raw logs
Native Tests - Windows - hibernate-validator ⚠️ Check → Logs Raw logs
Native Tests - gRPC ⚠️ Check → Logs Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 16 #

📦 extensions/arc/deployment

io.quarkus.arc.test.config.ConfigPropertiesTest. - More details - Source on GitHub

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 4, 2021

I think that having the OTLP Exporter in the guide would be preferable since it is the open standard protocol and the collector can export it later to whatever people want. And right now the difference between the two exporters in terms of configuration are minimal, and we can even add a note that switching to the Jaeger protocol would be as easy as using the Jaeger exporter import instead of the OTLP.

@kenfinnigan
Copy link
Member

Sounds good, could you add that with this PR? Thanks

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 4, 2021

Yup, saw that we already have the OTEL guide (#17639), will tweak it =]

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 7, 2021

@kenfinnigan updated the docs and also created a PR for the quick-start (quarkusio/quarkus-quickstarts#877)

@kenfinnigan kenfinnigan added this to the 2.1 - main milestone Jun 7, 2021
@kenfinnigan kenfinnigan added triage/backport? triage/waiting-for-ci Ready to merge when CI successfully finishes and removed area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins labels Jun 7, 2021
@kenfinnigan
Copy link
Member

Thanks @luneo7!

Really appreciate the contribution.

One thing I hadn't thought of, until your doc and quickstart changes, is that we might want to move the jaeger exporter into the Quarkiverse as opposed to being a main extension. What do you think?

@luneo7
Copy link
Contributor Author

luneo7 commented Jun 7, 2021

I think it makes sense to move the Jaeger exporter to the Quarkiverse, since we already have everything to make it work in the main extensions and nothing "core" depends on it.

@kenfinnigan kenfinnigan merged commit 7d397f7 into quarkusio:main Jun 8, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jun 8, 2021
@gsmet gsmet modified the milestones: 2.1 - main, 2.0.0.Final Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants