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

support exact kind in OTLP metrics exporter #1309

Merged
merged 14 commits into from
Nov 18, 2020

Conversation

hstan
Copy link
Contributor

@hstan hstan commented Nov 6, 2020

When using exactSelector we found that the OTLP exporter does not handle array aggregator's kind (ExactKind).
This PR tries to fix it.

@hstan hstan closed this Nov 6, 2020
@hstan hstan reopened this Nov 6, 2020
@hstan
Copy link
Contributor Author

hstan commented Nov 6, 2020

@jmacd seen from the failed test, I noticed it's explicitly asserted this shuold not be supported https://github.com/open-telemetry/opentelemetry-go/blob/master/exporters/otlp/internal/transform/metric_test.go#L349

I'm not sure why is that, does this change makes sense at all?

exporters/otlp/internal/transform/metric.go Outdated Show resolved Hide resolved
exporters/otlp/internal/transform/metric.go Outdated Show resolved Hide resolved
@MrAlias
Copy link
Contributor

MrAlias commented Nov 9, 2020

@jmacd seen from the failed test, I noticed it's explicitly asserted this shuold not be supported https://github.com/open-telemetry/opentelemetry-go/blob/master/exporters/otlp/internal/transform/metric_test.go#L349

I'm not sure why is that, does this change makes sense at all?

The change seems to make sense to me. If you configure the metrics telemetry pipeline to send exact values they should not error the exporter. The tests will need to be updated as part of this PR.

require.Error(t, err)
require.Nil(t, mpb)
require.True(t, errors.Is(err, ErrUnimplementedAgg))

@codecov
Copy link

codecov bot commented Nov 10, 2020

Codecov Report

Merging #1309 (910bbd5) into master (c857a3d) will increase coverage by 0.0%.
The diff coverage is 85.3%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1309   +/-   ##
======================================
  Coverage    77.5%   77.5%           
======================================
  Files         125     125           
  Lines        6090    6131   +41     
======================================
+ Hits         4720    4753   +33     
- Misses       1119    1127    +8     
  Partials      251     251           
Impacted Files Coverage Δ
exporters/otlp/internal/transform/metric.go 80.1% <85.3%> (+<0.1%) ⬆️

@hstan hstan requested a review from MrAlias November 10, 2020 07:30
Copy link
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

👍

@hstan
Copy link
Contributor Author

hstan commented Nov 11, 2020

@MrAlias
I think the build failed due to a flaky test, I've no permission to rerun (passed locally)

@jmacd
Copy link
Contributor

jmacd commented Nov 11, 2020

(Sorry all, I will catch up with this PR later today!!)

@jmacd
Copy link
Contributor

jmacd commented Nov 12, 2020

To be explicit, it is sensible and correct to expose exact values as Gauge data points.
We will eventually have a specification that states this behavior. 😀
Thanks 💪.

@jmacd
Copy link
Contributor

jmacd commented Nov 12, 2020

I see a connection with open-telemetry/opentelemetry-proto#188. There's no way to know that the resulting Gauge data points represent raw data, but I would like there to be.

@Aneurysm9
Copy link
Member

Apologies, this appears to be tripped up by a change to package structure, likely #1321.

@Aneurysm9 Aneurysm9 merged commit fd3c82b into open-telemetry:master Nov 18, 2020
@MrAlias MrAlias mentioned this pull request Nov 20, 2020
AzfaarQureshi pushed a commit to open-o11y/opentelemetry-go that referenced this pull request Dec 3, 2020
* support exact kind in OTLP metrics exporter

* add change log

* rename function

* inline start time and end time variables

* fix test

* add test for exact int data points

* add test for exact float data points

* use newly introduced number package for numbers according to upstream change

* fix package ref
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.

4 participants