Skip to content

OpenTelemetry emitter extension #12015

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

Merged
merged 5 commits into from
Jan 15, 2022

Conversation

IvanVan
Copy link
Contributor

@IvanVan IvanVan commented Dec 2, 2021

Description

OpenTelemetry extension for distributed traces.
Use Druid emitter functionality to ingest metrics and create spans from query/time metrics. See emitter for more information
Use context field for propagation. See druid context for more information.

image (2)

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • been tested in a test Druid cluster.

@FrankChen021
Copy link
Member

@IvanVan Thanks for your contribution.

For distributed tracing, I have been working on a side project by using java agent. This project not only collects metrics from any java processes, but also automatically injects opentelemetry standard tracing context in HTTP request to trace the call chain without any changes on Druid side.

Although it's still under development, I think you may be interested in it.

Here're some screenshots from my project.

image

image

image

image

image

@IvanVan IvanVan changed the title Add OpenTelemetry emitter extension OpenTelemetry emitter extension Dec 7, 2021
@xvrl
Copy link
Member

xvrl commented Dec 8, 2021

@FrankChen021 I agree we can gain a lot by deploying the agent as well. While it does get us visibility into more things, it doesn't get us access to the query context metadata which is useful to get into the trace context.

Ultimately I think we want those two approaches to converge and and work better together, so this is a starting point.
Another reason to have this extension is to give us a place where we can expose all the other druid metrics via opentelemetry.

Copy link
Member

@xvrl xvrl left a comment

Choose a reason for hiding this comment

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

I have already reviewed this code internally, and we've rolled this out to one of our clusters, so this LGTM. It would however be valuable to get any other high level feedback or suggestions from the community.

Comment on lines +45 to +46
"traceparent",
"tracestate"
Copy link
Member

Choose a reason for hiding this comment

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

nit, the docs mention traceparent, but they don't mention anything about tracestate or how it's being used. Can we add some details to clarify that?

@FrankChen021
Copy link
Member

FrankChen021 commented Jan 13, 2022

@xvrl Sorry for the late response. It's not a hard thing for the agent to get the data in the query context, Here's an example:
image
image

BWT, I'm not giving objections on the merging of this PR.

Copy link
Member

@asdf2014 asdf2014 left a comment

Choose a reason for hiding this comment

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

👍 LGTM, thanks a lot for your contribution

@asdf2014 asdf2014 merged commit 6a93872 into apache:master Jan 15, 2022
@jihoonson
Copy link
Contributor

jihoonson commented Jan 15, 2022

[ERROR] Failed to execute goal de.thetaphi:forbiddenapis:3.1:check (default-cli) on project opentelemetry-emitter: Parsing signatures failed: Method not found while parsing signature: com.google.common.util.concurrent.MoreExecutors#sameThreadExecutor() -> [Help 1]

I'm not sure how this PR passed Travis CI. But it broke the forbidden api checks. This seems like because of the different guava version that this extension uses. Any suggestions on how to fix this?

@jihoonson
Copy link
Contributor

This is a blocker and we should fix it as soon as possible since it is blocking every PR.

@asdf2014
Copy link
Member

@jihoonson How about this solution #12158

@asdf2014
Copy link
Member

@jihoonson At present, because of this #12034 PR, the failOnUnresolvableSignatures option has been deleted. After adding the old parameters back, there is no problem anymore. And the current PR's Travis runs before #12034, which explains why the current PR can succeed, but fails after the merge 😂

@abhishekagarwal87 abhishekagarwal87 added this to the 0.23.0 milestone May 11, 2022
sachinsagare pushed a commit to sachinsagare/druid that referenced this pull request Nov 7, 2022
* Add OpenTelemetry emitter extension

* Fix build

* Fix checkstyle

* Add used undeclared dependencies

* Ignore unused declared dependencies

(cherry picked from commit 6a93872)
Signed-off-by: ssagare <[email protected]>
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.

6 participants