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

feat: adds metrics processor component and topology test #270

Merged
merged 4 commits into from
Oct 14, 2021

Conversation

kotharironak
Copy link
Contributor

Description

As part of building a metrics pipeline, this PR adds the new metrics process component.
Ref : hypertrace/hypertrace#297

Testing

Have added the topology test which ingests the gauge metric and verify.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #270 (222f440) into main (31cf9fd) will decrease coverage by 0.10%.
The diff coverage is 69.56%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #270      +/-   ##
============================================
- Coverage     80.29%   80.18%   -0.11%     
- Complexity     1166     1180      +14     
============================================
  Files           102      106       +4     
  Lines          4542     4588      +46     
  Branches        423      424       +1     
============================================
+ Hits           3647     3679      +32     
- Misses          696      709      +13     
- Partials        199      200       +1     
Flag Coverage Δ
unit 80.18% <69.56%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...va/org/hypertrace/ingester/HypertraceIngester.java 71.92% <0.00%> (-2.62%) ⬇️
...hypertrace/metrics/processor/OtlpMetricsSerde.java 63.63% <63.63%> (ø)
.../hypertrace/metrics/processor/MetricsEnricher.java 75.00% <75.00%> (ø)
...ypertrace/metrics/processor/MetricsNormalizer.java 75.00% <75.00%> (ø)
...hypertrace/metrics/processor/MetricsProcessor.java 76.00% <76.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 31cf9fd...222f440. Read the comment docs.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@Override
public KeyValue<byte[], ResourceMetrics> transform(byte[] key, ResourceMetrics value) {
// noop enricher for now
return new KeyValue<>(key, value);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the first iteration, we will be extracting metrics from traces, and they will already have enriched attributes as labels. We will enhance this later, for now, they are just pass through. If requires, we can check if the metrics contain API and service id, and if not, we can drop them.

implementation("org.hypertrace.core.kafkastreams.framework:kafka-streams-framework:0.1.21")

// serde (todo)
// implementation("io.confluent:kafka-streams-protobuf-serde:6.0.1")
Copy link
Contributor

@surajpuvvada surajpuvvada Oct 14, 2021

Choose a reason for hiding this comment

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

don't you need this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently, I am using internally defined OltpMetricsSerde. I was facing some issue with this so working as follow up item on this. For now, removed this.

try {
return ResourceMetrics.parseFrom(data);
} catch (InvalidProtocolBufferException e) {
throw new RuntimeException(e);
Copy link
Contributor

Choose a reason for hiding this comment

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

assuming an exception handler is part of config that can deal with any exceptions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this will be handle by default for deserialization exception handler - LogAndContinueExceptionHandler. So it will just be logged and such messages will be dropped.


schema.registry.url = "http://localhost:8081"
schema.registry.url = ${?SCHEMA_REGISTRY_URL}
value.subject.name.strategy = "io.confluent.kafka.serializers.subject.TopicRecordNameStrategy"
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it TopicRecordNameStrategy ? are there going to be different message types sent on the topic ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are there going to be different message types sent on the topic ?
No. Removed it.

@github-actions

This comment has been minimized.

Copy link
Contributor

@surajpuvvada surajpuvvada left a comment

Choose a reason for hiding this comment

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

lgtm

@kotharironak kotharironak merged commit 896081c into main Oct 14, 2021
@kotharironak kotharironak deleted the metrics-enricher branch October 14, 2021 17:52
@github-actions
Copy link

Unit Test Results

  71 files  +1    71 suites  +1   1m 9s ⏱️ +18s
381 tests +1  381 ✔️ +1  0 💤 ±0  0 ❌ ±0 

Results for commit 896081c. ± Comparison against base commit 31cf9fd.

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.

2 participants