-
Notifications
You must be signed in to change notification settings - Fork 767
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: Add transform functions for Collector Metric Exporter #1288
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1288 +/- ##
==========================================
- Coverage 93.42% 93.12% -0.30%
==========================================
Files 132 133 +1
Lines 3754 3841 +87
Branches 763 785 +22
==========================================
+ Hits 3507 3577 +70
- Misses 247 264 +17
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just looking quickly, can you move all related metrics stuff into new file something like transform_metrics
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes
@obecny Some of the functions in |
I would split all of those that has anything to do with metrics, if they need to use the for example |
@obecny Ahh ok makes sense, thanks! Should the files be |
sounds good :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Minor style issues that don't actually affect the behavior.
packages/opentelemetry-exporter-collector/src/transformMetrics.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/transformMetrics.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/transformMetrics.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great, I like the changes, just minor issues
return opentelemetryProto.metrics.v1.MetricDescriptorType.DOUBLE; | ||
} | ||
|
||
// TODO: Add Summary once implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create issue and then add @TODO (#ISSUE_NUMBER)
metric.descriptor.metricKind === MetricKind.VALUE_OBSERVER || | ||
metric.descriptor.metricKind === MetricKind.VALUE_RECORDER | ||
) { | ||
// TODO: Change once LastValueAggregator is implemented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure about this LastValueAggregator ? As recently I have removed LastValueAggregator in favour of MinMaxLastSumCountAggregator
which is default now for value and recorder observers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked within Opentelemetry-proto on what to do about temporality (link here), and I was told that LastValueAggregator
and ExactValueAggregator
have instantaneous temporality, whereas MinMaxLastSumCountAggregator
is delta.
However, within my code, I just use MinMaxLastSumCountAggregator
essentially as a LastValueAggregator
anyways, because I just use last
and ignore the min, max, sum, and count. Should I just change this to use instantaneous temporality and remove the todo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this correctly I think the way you did it for now is fine. We just have to watch the mentioned issue as it seems like this is not a final, (had the same with LastValueAggregator).
One question, do you have maybe a working example similar to this one -> https://github.com/open-telemetry/opentelemetry-js/tree/master/examples/collector-exporter-node with a docker etc. so it will be possible to see metrics ? |
@obecny Yeah, I have a working example that I've been testing my work on. I didn't use docker though, so I have to set that up. I just have a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great :)
Summary
This PR addresses Issue #1109.
Currently, the Collector Exporter only sends traces to the collector. In Python, Java, and Go, for example, metric exporting already exists for the collector. This PR adds that feature. I created two versions, one for Node and the other for browser. The node version uses gRPC, and the browser uses Beacon or XHR. It's up to date with all previous collector exporter commits.
What this PR does
This is the 5th part of this long PR.
This PR creates the metric versions of transform functions, transforming objects to valid formats for the collector.
These are more straightforward and are essentially direct copies of the trace versions of the functions.
These functions are more complicated, where I had to make decisions for converting types (e.g. determining the appropriate type and temporality for a given MetricKind)
One particular question I had was regarding
LastValueAggregator
. This was the one of the two aggregators (along withExactAggregator
) which uses instantaneous temporality. I saw this was recently removed. Does that mean that the Temporality is now never instantaneous?Next Steps
The next diffs will be as follows:
Testing
I added detailed unit tests. Additionally, I E2E tested my code. However, histogram aggregators aren't currently supported, so I could only create a unit test for it.