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

METRICS-4792 : OpenTelemetry Traces Protobuf InputFormat #129

Closed
wants to merge 22 commits into from

Conversation

kamal-narayan
Copy link
Member

Description

  • Add an OpenTelemetry Traces InputFormat so that Druid can ingest records that are in the form of an OpenTelemetry Traces Protobuf.

  • Note: This does not cover some fields in Otel Trace proto such as Events and Links. The extension as a whole is meant to be contributed upstream for further improvements and maintenance.


Key changed/added classes in this PR
  • Refactor common functionality between Metrics and Traces into abstract classes OpenTelemetryInputFormat / OpenTelemetryProtobufReader
  • OpenTelemetryTracesProtobufReader
  • OpenTelemetryTracesProtobufInputFormat

Testing

  • Unit tests
  • Add Benchmark test for traces
Benchmark                                  (instrumentationScopeCount)  (resourceSpanCount)  (spansCount)   Mode  Cnt       Score       Error  Units
OpenTelemetryTracesBenchmark.measureSerde                            1                    1             1  thrpt    5  290712.694 ± 16258.114  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    1             2  thrpt    5  179720.468 ±  3255.420  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    1             4  thrpt    5   97546.502 ±  2833.230  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    1             8  thrpt    5   54750.250 ±  1943.527  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    2             1  thrpt    5  145419.049 ±  3366.026  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    2             2  thrpt    5   90352.972 ±  3579.583  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    2             4  thrpt    5   48941.613 ±  5208.820  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    2             8  thrpt    5   27515.141 ±  3776.942  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    4             1  thrpt    5   72962.145 ±  2408.336  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    4             2  thrpt    5   45559.232 ±  1580.323  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    4             4  thrpt    5   26834.922 ±  1049.008  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    4             8  thrpt    5   13932.725 ±   739.088  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    8             1  thrpt    5   38448.021 ±  1102.249  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    8             2  thrpt    5   23787.015 ±   775.349  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    8             4  thrpt    5   13311.163 ±   299.108  ops/s
OpenTelemetryTracesBenchmark.measureSerde                            1                    8             8  thrpt    5    7073.504 ±   336.945  ops/s

  • Local testing

    1. Deployed druid locally.
    2. Ingested Kafka stream with Otel trace data.
    3. Verified the data in the data source.

This PR has:

  • been self-reviewed.
  • 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.

Config unit tests

Fix dimension test

Introduce config class

Fix style

Add serde test

Prior to unit testing

Fix style errors

Add some dimensions
Benchmark + Test coverage

Add Benchmark

Add Benchmark / Fix tests

Improved test coverage

Fix line breaks

Fix config builder

clean up test cases

fix distribution file
@kamal-narayan kamal-narayan requested review from a team as code owners February 13, 2023 10:38
@CLAassistant
Copy link

CLAassistant commented Feb 13, 2023

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

distribution/pom.xml Show resolved Hide resolved
Comment on lines +88 to +92
<dependency>
<groupId>nl.jqno.equalsverifier</groupId>
<artifactId>equalsverifier</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member

Choose a reason for hiding this comment

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

adding a whole new dependency for a single test seems a little heavyweight? Can we accomplish that without this additional dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Equals tests are required to meet the code coverage threshold. (More details in the comment below)

Given there are 10 fields to check, I think it would be better to use this library as it covers all the negative branch cases too.

Copy link
Member

Choose a reason for hiding this comment

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

ok, I wonder why we don't need it for metrics input format. It looks like we already use this dependency elsewhere in druid so I'm ok using that here.

@kamal-narayan kamal-narayan requested a review from xvrl February 17, 2023 15:39
@kamal-narayan kamal-narayan force-pushed the kamal/add_otel_trace_converter branch from 3682487 to 348c0be Compare March 21, 2023 00:21
@kamal-narayan kamal-narayan force-pushed the kamal/add_otel_trace_converter branch from b74a499 to c05a185 Compare March 21, 2023 16:11
@kamal-narayan kamal-narayan requested a review from xvrl March 21, 2023 16:14
@@ -212,9 +212,9 @@
<module>extensions-contrib/aliyun-oss-extensions</module>
<module>extensions-contrib/prometheus-emitter</module>
<module>extensions-contrib/opentelemetry-emitter</module>
<module>extensions-contrib/opentelemetry-extensions</module>
Copy link
Member Author

@kamal-narayan kamal-narayan Mar 21, 2023

Choose a reason for hiding this comment

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

Reason for this move up:

Semaphore build jobs were failing with the following error:

[ERROR] Failed to execute goal de.thetaphi:forbiddenapis:3.2:check (default-cli) on project druid-opencensus-extensions: 
Check for forbidden API calls failed while scanning class 'org.apache.druid.data.input.opencensus.protobuf.OpenCensusProtobufInputRowParser' 
(OpenCensusProtobufInputRowParser.java): java.lang.ClassNotFoundException: org.apache.druid.data.input.opentelemetry.protobuf.AbstractProtobufReader (while looking up details about referenced class 'org.apache.druid.data.input.opencensus.protobuf.OpenCensusProtobufReader') -> [Help 1]

It looked like the forbiddenapis check was failing because of some classpath issue, even though the compilation step was passing. I wasn't able to reproduce this failure consistently on my laptop (but failed every time in Semaphore)

This issue looks to be closely related.

I initially tried using the @SuppressForbidden annotation for the OpenCensusProtobufInputRowParser class and that led to the same error for the OpenCensusProtobufReader class.
Ultimately Semaphore jobs passed with this change. I'm not sure how this change in module ordering fixes up the forbiddenapis check.

Empty-Commit
@kamal-narayan kamal-narayan force-pushed the kamal/add_otel_trace_converter branch from f03e847 to 7f5e5ba Compare March 21, 2023 20:51
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.

3 participants