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: Collector Metric Exporter[2/x] Create CollectorMetricExporterBase #1258

Merged
merged 18 commits into from
Jul 6, 2020

Conversation

davidwitten
Copy link
Member

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 2nd part of this long PR.

This PR creates CollectorMetricExporterBase, modeled off CollectorTraceExporterBase. I also create unit tests and add two mock metrics to test with, a mock counter and a mock observer. The base class does nothing, so this won't break anything.

Next Steps

The next diffs will be as follows

  1. Add transform functions and tests
  2. E2E Metric exporter for Browser with tests
  3. E2E Metric exporter for Node with tests

@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #1258 into master will increase coverage by 0.01%.
The diff coverage is 91.48%.

@@            Coverage Diff             @@
##           master    #1258      +/-   ##
==========================================
+ Coverage   93.01%   93.03%   +0.01%     
==========================================
  Files         131      132       +1     
  Lines        3696     3735      +39     
  Branches      747      755       +8     
==========================================
+ Hits         3438     3475      +37     
- Misses        258      260       +2     
Impacted Files Coverage Δ
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <ø> (ø)
...orter-collector/src/CollectorMetricExporterBase.ts 89.74% <89.74%> (ø)
...porter-collector/src/CollectorTraceExporterBase.ts 94.87% <100.00%> (+5.12%) ⬆️

@davidwitten
Copy link
Member Author

This PR: #1254 reverted my proto change, so I'm doing it again.

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

Looks good thanks for the contribution

Copy link
Member

@naseemkullah naseemkullah left a comment

Choose a reason for hiding this comment

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

nit: hostname is one word, other than that and apparent lack of code coverage lgtm

@davidwitten
Copy link
Member Author

@naseemkullah Actually, I realized that this.hostname is also set in CollectorTraceExporterBase and is never used. Is it okay to just remove it from both?

@naseemkullah
Copy link
Member

naseemkullah commented Jul 2, 2020

@naseemkullah Actually, I realized that this.hostname is also set in CollectorTraceExporterBase and is never used. Is it okay to just remove it from both?

At a quick glance, imho, we can remove hostname but we should add host and port, which should be used with Node, whereas url with browser, since for reasons beyond my understanding they must be configured differently. @open-telemetry/javascript-maintainers please advise.

edit: It's just weird in Node's case that the value of url is not a url

@naseemkullah
Copy link
Member

All that to say I'm ok with removing hostname and (eventually adding host and port, probably not in this PR, if Node cannot be configured via url with a url).

@davidwitten
Copy link
Member Author

Thanks @naseemkullah! For now, I just changed hostName to hostname and added more unit tests. I'll make another PR to remove it and replace it with host/port.

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

As this i based already on CollectorTraceExporter can you please align name hostName in both places so it will be also hostname ? ignore I see you already did

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm
Nit: please rename CollectorExporter in tests into CollectorTraceExporter

@dyladan dyladan merged commit 46ce535 into open-telemetry:master Jul 6, 2020
@davidwitten davidwitten deleted the exporter branch July 6, 2020 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants