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.

Sign up for GitHub

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

Jump to bottom

Add OpenTelemetry Proto Metrics Reader Extension #17668

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add OpenTelemetry Proto Metrics Reader Extension #17668

wants to merge 5 commits into from

Conversation

suraj-goel
Copy link
Contributor

@suraj-goel suraj-goel commented Jan 27, 2025 •
edited
Loading

Fixes #17644.

Description

  • Added OpenTelemetry Proto Metrics Reader extension.
  • Benchmark results
Screenshot 2025-01-27 at 9 47 07 PM Screenshot 2025-01-27 at 9 51 37 PM

Release note

  • A new extension, druid-opentelemetry-extensions, has been added to enable ingestion of metrics in OpenTelemetry Protocol (OTLP) format.
  • This extension allows Druid to ingest metrics data directly from OpenTelemetry sources using the OTLP format, aligning Druid with industry-standard telemetry protocols.
  • To use this extension, refer to attached README

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • 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.
  • added integration tests.
  • been tested in a test Druid cluster.

github-advanced-security[bot]
github-advanced-security bot found potential problems Jan 27, 2025
View reviewed changes
...g/apache/druid/data/input/opentelemetry/protobuf/OpenTelemetryMetricsProtobufReaderTest.java Outdated
));

@Rule
public ExpectedException expectedException = ExpectedException.none();

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
ExpectedException.none
Loading
should be avoided because it has been deprecated.
@suraj-goel suraj-goel marked this pull request as ready for review January 27, 2025 16:40
@FrankChen021
Copy link
Member

We already an extensions named as opentelemetry-emitter which supports emitting tracing logs, and the new extension in this PR is druid-opentelemetry-extensions, which looks like an extensions that includes the emitter. I'm worried that this might cause some confusion.

Since the extension supports ingestion data in the opentelemetry proto format, maybe we can rename it as `druid-opentelemetry-metric-extensions' or sth else.

@suraj-goel
Copy link
Contributor Author

We already an extensions named as opentelemetry-emitter which supports emitting tracing logs, and the new extension in this PR is druid-opentelemetry-extensions, which looks like an extensions that includes the emitter. I'm worried that this might cause some confusion.

Since the extension supports ingestion data in the opentelemetry proto format, maybe we can rename it as `druid-opentelemetry-metric-extensions' or sth else.

Right now, this extension supports ingestion of protobuf OTLP Metrics events.
The idea is that we can extend the same extension further to include OTLP traces and spans.

Let me know if this makes sense.

@FrankChen021
Copy link
Member

From the general software design principle, it makes sense to leave a room to add trace and span logs in future.

However, from reality, I would like to argue that if Druid is the right choice for span logs. For span logs, scan queries or point queries are mainly involved, and because Druid lacks rerverted index, bloom filter index, serving these needs would not be a good idea. And now, i also don't see Druid has any plan on the the index or storage layer to support these scenarios.

@suraj-goel
Copy link
Contributor Author

@FrankChen021, As per your suggestion, I have renamed the extension.

cryptoe
cryptoe reviewed Mar 6, 2025
View reviewed changes
...a/org/apache/druid/data/input/opentelemetry/protobuf/OpenTelemetryMetricsProtobufReader.java Outdated
});
break;
}
// TODO Support HISTOGRAM and SUMMARY metrics
Copy link
Contributor

Choose a reason for hiding this comment

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

We donot add todo's in the production code.
Just mention that we donot support histogram and summary and document it as part of the readme.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the ToDos and added the supported metrics and data types in readme.

...a/org/apache/druid/data/input/opentelemetry/protobuf/OpenTelemetryMetricsProtobufReader.java Outdated
case STRING_VALUE:
return value.getStringValue();

// TODO: Support KVLIST_VALUE, ARRAY_VALUE and BYTES_VALUE
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the ToDos and added the supported metrics and data types in readme.

...g/apache/druid/data/input/opentelemetry/protobuf/OpenTelemetryMetricsProtobufReaderTest.java
@@ -0,0 +1,442 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Could you please add a proto file to the test/resources and try to read it from from there ? It can be a new test case as well .
  2. Add tests cases for unsupported columns/data types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Added
  2. The test cases are already present for unsupported metric/data types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cryptoe cryptoe cryptoe left review comments

Assignees
No one assigned
Labels
Area - Dependencies Area - Ingestion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Druid Extension to read messages in OpenTelemetry Metric Protobuf format
3 participants
@suraj-goel @FrankChen021 @cryptoe