feat: Add transform functions for Collector Metric Exporter

[davidwitten]

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.

• toCollectorLabels
• groupMetricsByResourceAndLibrary
• toCollectorExportMetricServiceRequest
• toCollectorResourceMetrics
• toCollectorInstrumentationLibraryMetrics

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)

• toCollectorType
• toCollectorTemporality (source for how I did it)
• toCollectorMetricDescriptor
• toSingularPoint
• toHistogramPoint

One particular question I had was regarding LastValueAggregator. This was the one of the two aggregators (along with ExactAggregator) 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:

1. E2E Metric exporter for Browser with tests
2. E2E Metric exporter for Node with tests

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.

[codecov]

Codecov Report

Merging #1288 into master will decrease coverage by 0.29%.
The diff coverage is 80.89%.

@@            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     

Impacted Files	Coverage Δ
...ages/opentelemetry-exporter-collector/src/types.ts	100.00% <ø> (ø)
...lemetry-exporter-collector/src/transformMetrics.ts	80.68% <80.68%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts	96.29% <100.00%> (-0.05%)	⬇️

[obecny]

just looking quickly, can you move all related metrics stuff into new file something like transform_metrics ?

[dyladan]

Thanks for the changes

[davidwitten]

@obecny Some of the functions in transform.ts are general purpose (e.g. toCollectorResource, toCollectorAttributes, which are used for both span + metrics). Others are span functions only. Finally, the ones I added are metric functions. I'm worried that splitting the file up into 3 files will be confusing. What do you think?

[obecny]

@obecny Some of the functions in transform.ts are general purpose (e.g. toCollectorResource, toCollectorAttributes, which are used for both span + metrics). Others are span functions only. Finally, the ones I added are metric functions. I'm worried that splitting the file up into 3 files will be confusing. What do you think?

I would split all of those that has anything to do with metrics, if they need to use the for example toCollectorAttributes or toCollectorResource than you can import it. The reason is that this file may soon become bigger, but also this file is used by web and by node. The web might not need metrics in most cases so I would like to avoid importing functions that are not needed on web. Also testing file will become bigger, so I wouldn't worry of having more smaller files then one big. And finally we already have exporter for metrics and for spans, so if something is not needed for span exporter but only in metric exporter I would keep it separately and vice versa. WDYT ?

[davidwitten]

@obecny Ahh ok makes sense, thanks! Should the files be transform.ts and transformMetrics.ts?

[obecny]

@obecny Ahh ok makes sense, thanks! Should the files be transform.ts and transformMetrics.ts?

sounds good :)

[dyladan]

LGTM. Minor style issues that don't actually affect the behavior.

[obecny]

this looks great, I like the changes, just minor issues

[obecny]

please create issue and then add @TODO (#ISSUE_NUMBER)

[obecny]

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

[davidwitten]

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?

[obecny]

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).

[obecny]

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 ?

[davidwitten]

@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 config.yaml file for my collector. Once this is fully submitted, I can make a PR uploading my example and documentation on how to run it.

[obecny]

great :)