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: adding json over http for collector exporter #1247

Merged
merged 20 commits into from
Jul 6, 2020

Conversation

obecny
Copy link
Member

@obecny obecny commented Jun 25, 2020

Which problem is this PR solving?

Short description of the changes

  • Adds json over http for node for Collector Exporter

@obecny obecny added the enhancement New feature or request label Jun 25, 2020
@obecny obecny self-assigned this Jun 25, 2020
@obecny obecny added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 25, 2020
@codecov
Copy link

codecov bot commented Jun 25, 2020

Codecov Report

Merging #1247 into master will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1247      +/-   ##
==========================================
+ Coverage   92.86%   93.08%   +0.21%     
==========================================
  Files         130      133       +3     
  Lines        3546     3759     +213     
  Branches      728      760      +32     
==========================================
+ Hits         3293     3499     +206     
- Misses        253      260       +7     
Impacted Files Coverage Δ
...ages/opentelemetry-exporter-collector/src/types.ts 100.00% <ø> (ø)
...porter-collector/src/CollectorTraceExporterBase.ts 94.87% <100.00%> (ø)
.../opentelemetry-exporter-collector/src/transform.ts 96.34% <100.00%> (ø)
...kages/opentelemetry-exporter-collector/src/util.ts 100.00% <100.00%> (ø)
packages/opentelemetry-plugin-grpc/src/grpc.ts 96.79% <0.00%> (ø)
packages/opentelemetry-plugin-grpc/src/utils.ts 93.75% <0.00%> (ø)

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

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

lgtm, little nitpicks but no blocker

@obecny obecny requested a review from naseemkullah June 26, 2020 12:51
Copy link
Member

@mwear mwear left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of comments / questions.

@obecny obecny requested a review from mwear June 26, 2020 21:49
@obecny
Copy link
Member Author

obecny commented Jun 30, 2020

@open-telemetry/javascript-maintainers ^^

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.

Most of my comments are small/minor - going to provide LGTM and hope you can address prior to merging.

const exporter = new CollectorTraceExporter({
serviceName: 'basic-service',
url: address,
// headers: {
Copy link
Member

Choose a reason for hiding this comment

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

Why this is commented out? Custom headers and metadata feature released in 0.9.0.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is an example to be able to test it or not

@obecny obecny merged commit f0a3dba into open-telemetry:master Jul 6, 2020
@obecny obecny deleted the collector_json branch July 6, 2020 19:37
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
enhancement New feature or request size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants