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][1/x] Rename CollectorExporter to CollectorTraceExporter #1256

Merged
merged 3 commits into from
Jun 29, 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

I previously made a long PR. I'm splitting it up into multiple PRs to make it easier to review.

This PR renames CollectorExporter to CollectorTraceExporter. Additionally, I move CollectorExporterConfig to the types file, as it will be reused by the Collector Metric Exporter.

Next Steps

The next diffs will be as follows (Thanks to @mayurkale22 for the suggestion):

  1. Add CollectorMetricExporterBase class
  2. E2E Metric exporter for Browser with tests
  3. E2E Metric exporter for Node with tests
  4. Update examples and Documentation etc.

@codecov
Copy link

codecov bot commented Jun 29, 2020

Codecov Report

Merging #1256 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1256   +/-   ##
=======================================
  Coverage   93.29%   93.29%           
=======================================
  Files         126      126           
  Lines        3597     3597           
  Branches      724      724           
=======================================
  Hits         3356     3356           
  Misses        241      241           
Impacted Files Coverage Δ
.../opentelemetry-exporter-collector/src/transform.ts 96.34% <ø> (ø)
...porter-collector/src/CollectorTraceExporterBase.ts 89.74% <100.00%> (ø)
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <100.00%> (ø)

Copy link
Member

@mayurkale22 mayurkale22 left a comment

Choose a reason for hiding this comment

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

Thanks for chopping the PR, overall LGTM.

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.

Much easier to review. Naming change LGTM

@dyladan dyladan removed the enhancement New feature or request label Jun 29, 2020
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.

All examples will stop working when this PR will be merged:

  • examples/collector-exporter-node
  • examples/tracer-web
    So I think they should be updated as well.
    Also the master should always be in state "ready to release" so I think the documentation should be updated as well. Otherwise we will be blocked from releasing or making more updates to collector (currently opened PR for json over http and next will be coming for proto over http)

@dyladan
Copy link
Member

dyladan commented Jun 29, 2020

All examples will stop working when this PR will be merged:

  • examples/collector-exporter-node
  • examples/tracer-web
    So I think they should be updated as well.
    Also the master should always be in state "ready to release" so I think the documentation should be updated as well. Otherwise we will be blocked from releasing or making more updates to collector (currently opened PR for json over http and next will be coming for proto over http)

Think @mayurkale22 asked him to break it up this way, but I generally agree that if it does not blow up the PR and make it too big, the docs and examples changes should probably also be included. Since this is just a naming change it shouldn't be too huge of a difference with them.

@obecny
Copy link
Member

obecny commented Jun 29, 2020

All examples will stop working when this PR will be merged:

  • examples/collector-exporter-node
  • examples/tracer-web
    So I think they should be updated as well.
    Also the master should always be in state "ready to release" so I think the documentation should be updated as well. Otherwise we will be blocked from releasing or making more updates to collector (currently opened PR for json over http and next will be coming for proto over http)

Think @mayurkale22 asked him to break it up this way, but I generally agree that if it does not blow up the PR and make it too big, the docs and examples changes should probably also be included. Since this is just a naming change it shouldn't be too huge of a difference with them.

yes the examples and doc for it - it should be just renaming, nothing more

@mayurkale22
Copy link
Member

Sure, makes sense to me.

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.

Why the files from protos folder are being updated ?, I don't see to which revision the folder were updated, is this intentional ?

@davidwitten
Copy link
Member Author

davidwitten commented Jun 29, 2020

@obecny Thanks, I'll add documentation. Updating protos is necessary, otherwise it won't work with the latest version of the collector. For example, in the previous version (which was from March I believe), the proto still used OpenCensus names (e.g. Gauge). I'll update the documentation for the protos as well.

@obecny
Copy link
Member

obecny commented Jun 29, 2020

@obecny Thanks, I'll add documentation. Updating protos is necessary, otherwise it won't work with the latest version of the collector. For example, in the previous version (which was from March I believe), the proto still used OpenCensus names (e.g. Gauge). I'll update the documentation for the protos as well.

Ok but which sha commit are you updating to ? "Normally" when you update the submodule to certain revision you should see the changes under folder and then it will have only a different sha, nothing more, but here I see a files instead of sha so not sure how did you update that ?

@obecny
Copy link
Member

obecny commented Jun 29, 2020

I'm talking about this https://imgur.com/a/VG14xcE

@davidwitten
Copy link
Member Author

@obecny I thought I just followed the directions in the readme, I'll try to do it again.

@dyladan
Copy link
Member

dyladan commented Jun 29, 2020

I'm talking about this imgur.com/a/VG14xcE

This is just new UI for github submodule updates which gives you some idea of how big of a change you are introducing. It is the same thing "under the hood".

@obecny
Copy link
Member

obecny commented Jun 29, 2020

I'm talking about this imgur.com/a/VG14xcE

This is just new UI for github submodule updates which gives you some idea of how big of a change you are introducing. It is the same thing "under the hood".

ok thx :),

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, thx for updating examples and docs :)

@mayurkale22 mayurkale22 merged commit a370a47 into open-telemetry:master Jun 29, 2020
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
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.

None yet

5 participants