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 pull based metrics exporter #271

Merged
merged 9 commits into from
Oct 28, 2021
Merged

feat: adds pull based metrics exporter #271

merged 9 commits into from
Oct 28, 2021

Conversation

kotharironak
Copy link
Contributor

Description

As part of this ticket - hypertrace/hypertrace#304 - this PR adds,

  • adds initial pull-based exporter which converts otlp proto metrics format to Prometheus exposition format
  • as of now, it does auto-commit, in the second iteration, we will move to mgmt of commit (this needs changes in service framework)

Testing

  • working on adding tests

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 20, 2021

Codecov Report

Merging #271 (24c920a) into main (8fa4843) will decrease coverage by 1.23%.
The diff coverage is 43.75%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #271      +/-   ##
============================================
- Coverage     80.46%   79.22%   -1.24%     
- Complexity     1209     1230      +21     
============================================
  Files           106      110       +4     
  Lines          4663     4823     +160     
  Branches        435      439       +4     
============================================
+ Hits           3752     3821      +69     
- Misses          710      798      +88     
- Partials        201      204       +3     
Flag Coverage Δ
unit 79.22% <43.75%> (-1.24%) ⬇️

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

Impacted Files Coverage Δ
...trace/metrics/exporter/MetricsExporterService.java 0.00% <0.00%> (ø)
...etrics/exporter/consumer/MetricsKafkaConsumer.java 0.00% <0.00%> (ø)
...va/org/hypertrace/ingester/HypertraceIngester.java 63.23% <18.18%> (-8.70%) ⬇️
...exporter/utils/OtlpProtoToMetricDataConverter.java 89.83% <89.83%> (ø)
...ics/exporter/producer/InMemoryMetricsProducer.java 100.00% <100.00%> (ø)
...race/core/rawspansgrouper/TraceEmitPunctuator.java 74.56% <0.00%> (-0.88%) ⬇️

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 8fa4843...24c920a. Read the comment docs.

@github-actions

This comment has been minimized.

@kotharironak kotharironak marked this pull request as ready for review October 20, 2021 16:18
@github-actions

This comment has been minimized.

@kotharironak
Copy link
Contributor Author

@surajpuvvada @laxmanchekka @rish691 I am still working on this PR for adding tests. But, open for review for early inputs.

Map<String, Object> baseProperties = new HashMap<>();
baseProperties.put(ConsumerConfig.GROUP_ID_CONFIG, "hypertrace-metrics-exporter");
baseProperties.put(ConsumerConfig.ENABLE_AUTO_COMMIT_CONFIG, "true");
baseProperties.put(ConsumerConfig.AUTO_COMMIT_INTERVAL_MS_CONFIG, "1000");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the description that we will handle the manual coming in the second iteration.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

private final KafkaConsumer<byte[], byte[]> consumer;
private final InMemoryMetricsProducer inMemoryMetricsProducer;

public MetricsKafkaConsumer(Config config, InMemoryMetricsProducer inMemoryMetricsProducer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am thinking of adding TestContainer based test as a follow-up. Added test for an important one.

@kotharironak
Copy link
Contributor Author

@rish691 @surajpuvvada @laxmanchekka can you have a look?

}

public void run() {
while (true && !Thread.currentThread().isInterrupted()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is true needed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will address this in follow up PR.

List<MetricData> metricData = OtlpProtoToMetricDataConverter.toMetricData(rm);
boolean result = false;
while (!result) {
result = inMemoryMetricsProducer.addMetricData(metricData);
Copy link
Contributor

Choose a reason for hiding this comment

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

if in memory queue is full then wait and loop until the queue is drained during a prometheus pull ?

Copy link
Contributor Author

@kotharironak kotharironak Oct 28, 2021

Choose a reason for hiding this comment

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

Yes, we are waiting for enough space to add all metrics data associated with current ResourceMetrics. this can delay the poll at least for pull duration from Prometheus. So, for now, we need to set poll session time out higher than Prometheus poll (1/3 of session time out).

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

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit be50ac2 into main Oct 28, 2021
@kotharironak kotharironak deleted the metrics-exporter branch October 28, 2021 05:34
@github-actions
Copy link

Unit Test Results

  73 files  +2    73 suites  +2   1m 1s ⏱️ -10s
386 tests +3  386 ✔️ +3  0 💤 ±0  0 ❌ ±0 

Results for commit be50ac2. ± Comparison against base commit 8fa4843.

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