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

Add OTLP metrics exporter #2092

Merged

Conversation

alanwest
Copy link
Member

This is a first cut at an OTLP metrics exporter. It compiles, so it must work. No, but really, I'm just manually testing things out right now. I need to add some tests to this PR.

There's some code duplication between this and the trace exporter. I'll plan to handle this by refactoring things on the main branch and then merging main -> metrics.

I can add some benchmarks in a follow up PR.

@alanwest alanwest requested a review from a team June 17, 2021 23:29
@alanwest alanwest marked this pull request as draft June 17, 2021 23:30
@codecov
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #2092 (7d60ca6) into metrics (a1f6f42) will decrease coverage by 2.58%.
The diff coverage is 1.70%.

Impacted file tree graph

@@             Coverage Diff             @@
##           metrics    #2092      +/-   ##
===========================================
- Coverage    77.95%   75.36%   -2.59%     
===========================================
  Files          212      215       +3     
  Lines         6658     6893     +235     
===========================================
+ Hits          5190     5195       +5     
- Misses        1468     1698     +230     
Impacted Files Coverage Δ
...tryProtocol/Implementation/MetricItemExtensions.cs 0.00% <0.00%> (ø)
...etryProtocol/OtlpMetricExporterHelperExtensions.cs 0.00% <0.00%> (ø)
...orter.OpenTelemetryProtocol/OtlpMetricsExporter.cs 0.00% <0.00%> (ø)
...tation/OpenTelemetryProtocolExporterEventSource.cs 55.00% <100.00%> (+5.00%) ⬆️
...orter.OpenTelemetryProtocol/OtlpExporterOptions.cs 100.00% <100.00%> (ø)
...emetry.Api/Internal/OpenTelemetryApiEventSource.cs 82.35% <0.00%> (+2.94%) ⬆️

@cijothomas
Copy link
Member

@alanwest Does the OTLPCollector have ability to print the metrics to console? or can it act as prometheus scrape target?

@alanwest
Copy link
Member Author

alanwest commented Jun 18, 2021

@alanwest Does the OTLPCollector have ability to print the metrics to console?

Yep, it does. Added an option to the metrics example app with instructions on how to run.

or can it act as prometheus scrape target?

The collector does have a prometheus receiver that can be configured to scrape an endpoint, but then this wouldn't be OTLP 😛.

@@ -19,3 +19,7 @@
[assembly: InternalsVisibleTo("OpenTelemetry.Tests" + AssemblyInfo.PublicKey)]
[assembly: InternalsVisibleTo("DynamicProxyGenAssembly2" + AssemblyInfo.MoqPublicKey)]
[assembly: InternalsVisibleTo("Benchmarks" + AssemblyInfo.PublicKey)]

// TODO: Much of the metrics SDK is currently internal. These should be removed once the public API surface area for metrics is defined.
Copy link
Member

Choose a reason for hiding this comment

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

agree this is good for short term while we iron-out the public api .

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this now. The MetricItem and its associated classes must be public now. (Even though its highly likely they'll change, when we try for perf tweaks!)

@alanwest alanwest force-pushed the alanwest/metrics-otlp-exporter branch from 5f6c690 to 741ee92 Compare July 8, 2021 18:26
/// <summary>
/// Gets or sets the metric export interval.
/// </summary>
public int MetricExportInterval { get; set; } = 1000;
Copy link
Member Author

Choose a reason for hiding this comment

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

For now just putting these new metric related options here, though it might make sense to make an OtlpMetricsExporterOptions that derives from OtlpExporterOptions. Though also there are things like BatchExportProcessorOptions on this class that may not make sense in the context of metrics.

@alanwest alanwest marked this pull request as ready for review July 17, 2021 00:19
@alanwest alanwest changed the title [WIP] Add OTLP metrics exporter Add OTLP metrics exporter Jul 17, 2021

var options = new OtlpExporterOptions();
configure?.Invoke(options);
return builder.AddMetricProcessor(new PushMetricProcessor(new OtlpMetricsExporter(options), options.MetricExportInterval, options.IsDelta));
Copy link
Member

Choose a reason for hiding this comment

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

❤️
OTLP allows both delta and cumulative, so its good to let user pick.
Once views, come, things will be bit more complex, but shouldn't be an issue in 1st release!

{
if (this.ProcessResource == null)
{
this.SetResource(this.ParentProvider.GetResource());
Copy link
Member

Choose a reason for hiding this comment

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

this might not be working now. but we can address it later.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

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

LGTM!

@cijothomas
Copy link
Member

Merging! We can add tests and more in future PRs!

@cijothomas cijothomas merged commit 5d6d400 into open-telemetry:metrics Jul 17, 2021
@alanwest alanwest deleted the alanwest/metrics-otlp-exporter branch July 19, 2021 21:01
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