-
Notifications
You must be signed in to change notification settings - Fork 806
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
Conversation
Codecov Report
@@ 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
|
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, little nitpicks but no blocker
packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.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.
LGTM. Just a couple of comments / questions.
packages/opentelemetry-exporter-collector/src/platform/browser/CollectorExporter.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.ts
Outdated
Show resolved
Hide resolved
packages/opentelemetry-exporter-collector/src/platform/node/CollectorExporter.ts
Outdated
Show resolved
Hide resolved
@open-telemetry/javascript-maintainers ^^ |
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.
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: { |
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.
Why this is commented out? Custom headers and metadata feature released in 0.9.0
.
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 is an example to be able to test it or not
Which problem is this PR solving?
Short description of the changes