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

Fix OpenTelemetry late bound span processors #18686

Merged
merged 1 commit into from Jul 15, 2021

Conversation

kenfinnigan
Copy link
Member

- Fixes quarkusio#18575
- Changes default in `LateBoundBatchSpanProcessor` for `isEndRequired()` to `true`. In multiple span processor situation, this is necessary to ensure spans are reported
@kenfinnigan
Copy link
Member Author

@hacst could you verify this fixes it for you?

I've verified locally and I think it's good, but would be great to have additional verification

@ebullient
Copy link
Contributor

@hacst could you verify this fixes it for you?

I've verified locally and I think it's good, but would be great to have additional verification

is there any way to add this to automated tests?

@kenfinnigan
Copy link
Member Author

It would require an example with multiple exporters present, so likely a whole other module

@hacst
Copy link
Contributor

hacst commented Jul 14, 2021

Thanks a lot for the fix. I'll have to look into how to build a custom quarkus version to test this (or are there Nightly builds in some repository I am unaware of?). Will try to do so tomorrow.

@kenfinnigan
Copy link
Member Author

@hacst not sure if there are nightlies or not.

The easiest way is to check out either the PR, or my branch with the fix, and then run mvn -Dquickly to build it. Update any example to use 999-SNAPSHOT and quarkus-bom instead of quarkus-universe-bom and I think that's it

@gsmet gsmet merged commit 82ae780 into quarkusio:main Jul 15, 2021
@hacst
Copy link
Contributor

hacst commented Jul 15, 2021

I verified that a Quarkus with this fix resolves the issue in my testing. Thanks a lot.

@hacst
Copy link
Contributor

hacst commented Jul 16, 2021

@kenfinnigan while the issue is gone (export works) I still see the warning in the log on startup (tried only with dev server):

2021-07-16 09:50:38,849 WARN  [io.qua.ope.exp.jae.run.LateBoundBatchSpanProcessor] (Quarkus Main Thread) No BatchSpanProcessor delegate specified, no action taken.

Is that expected?

@kenfinnigan
Copy link
Member Author

@hacst Likely yes, as all it means is the first time the late bound is accessed the delegate isn't present, probably during instantiation of the MultiSpanProcessor, but subsequent span processing is fine because the delegate is then set.

It might be possible to tweak things in the multi processor situation to not output the message, but it would require investigation and may not be possible

@gsmet gsmet modified the milestones: 2.1.0.CR1, 2.0.3.Final Jul 16, 2021
@kenfinnigan kenfinnigan deleted the fix-multiple-exporters branch July 19, 2021 12:36
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.

Custom OpenTelemetry SpanExporter breaks Jaeger exporter
4 participants