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.

Sign up for GitHub

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

Jump to bottom

feat(opentelemetry-exporter-jaeger): http sender #965

Merged
merged 24 commits into from
Jun 10, 2020
Merged

feat(opentelemetry-exporter-jaeger): http sender #965

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
3942759
feat(opentelemetry-exporter-jaeger): http sender
leonardodalcin Apr 13, 2020
0408488
fix: linter
leonardodalcin Apr 13, 2020
5de0fba
Merge branch 'master' into http-sender
leonardodalcin Apr 14, 2020
256eb0e
fix(opentelemetry-exporter-jaeger): adds header to avoid infinity loop
leonardodalcin Apr 17, 2020
cf5fe82
Merge branch 'master' into http-sender
leonardodalcin Apr 17, 2020
2ef75b5
test(opentelemetry-exporter-jaeger): verify http sender usage
leonardodalcin Apr 17, 2020
a2ce058
Merge remote-tracking branch 'origin/http-sender' into http-sender
leonardodalcin Apr 17, 2020
0e1abfe
Merge branch 'master' into http-sender
leonardodalcin Apr 17, 2020
7e64566
refactor(opentelemetry-exporter-jaeger): checks if endpoint is setten
leonardodalcin Apr 20, 2020
0a15276
Merge branch 'master' into http-sender
leonardodalcin May 8, 2020
5620e16
feat(opentelemetry-exporter-jaeger): adds nock as dev dependency
leonardodalcin May 8, 2020
90f919e
test(opentelemetry-exporter-jaeger): adds tests to check req header
leonardodalcin May 8, 2020
b595b54
Merge remote-tracking branch 'origin/http-sender' into http-sender
leonardodalcin May 8, 2020
38dc850
fix: tests variable usage
leonardodalcin May 8, 2020
0235711
Merge branch 'master' into http-sender
mayurkale22 May 27, 2020
0d8b202
Merge branch 'master' into http-sender
leonardodalcin May 27, 2020
2c75792
refactor(opentelemetry-exporter-jaeger): removes config parameter change
leonardodalcin Jun 3, 2020
a528a1e
Merge branch 'master' into http-sender
leonardodalcin Jun 3, 2020
d284387
fix: linter
leonardodalcin Jun 3, 2020
8d51800
fix: env var was never called
leonardodalcin Jun 3, 2020
abcfcc7
Merge remote-tracking branch 'origin/http-sender' into http-sender
leonardodalcin Jun 3, 2020
9398254
Merge branch 'master' into http-sender
leonardodalcin Jun 9, 2020
69d3f13
Merge branch 'master' into http-sender
dyladan Jun 10, 2020
2e94c67
Merge branch 'master' into http-sender
mayurkale22 Jun 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion packages/opentelemetry-exporter-jaeger/src/jaeger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,17 @@ export class JaegerExporter implements SpanExporter {
typeof config.flushTimeout === 'number' ? config.flushTimeout : 2000;

config.host = config.host || process.env.JAEGER_AGENT_HOST;

config.endpoint = config.endpoint || process.env.JAEGER_ENDPOINT;
config.username = config.username || process.env.JAEGER_USER;
config.password = config.password || process.env.JAEGER_PASSWORD;
Copy link
Member

Choose a reason for hiding this comment

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

I woud prefer to verify that one of username/password or endpoint is defined and valid to avoid runtime errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 7e64566

Copy link
Member

Choose a reason for hiding this comment

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

With your commit, i think it's not possible to only use JAEGER_AGENT_ENDPOINT env var since you check for config.endpoint which should be undefined in this case. Is that normal ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If process.env.JAEGER_ENDPOINT is defined, then config.endpoint will also be. I don't think i understood what you meant.

// https://github.com/jaegertracing/jaeger-client-node#environment-variables
// By default, the client sends traces via UDP to the agent at localhost:6832. Use JAEGER_AGENT_HOST and
// JAEGER_AGENT_PORT to send UDP traces to a different host:port. If JAEGER_ENDPOINT is set, the client sends traces
// to the endpoint via HTTP, making the JAEGER_AGENT_HOST and JAEGER_AGENT_PORT unused. If JAEGER_ENDPOINT is secured,
// HTTP basic authentication can be performed by setting the JAEGER_USER and JAEGER_PASSWORD environment variables.
this._sender = config.endpoint
? new jaegerTypes.HTTPSender(config)
dyladan marked this conversation as resolved.
Show resolved Hide resolved
: new jaegerTypes.UDPSender(config);
this._sender = new jaegerTypes.UDPSender(config);
if (this._sender._client instanceof Socket) {
// unref socket to prevent it from keeping the process running
Expand Down
10 changes: 10 additions & 0 deletions packages/opentelemetry-exporter-jaeger/src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,13 @@ export interface ExporterConfig {
maxPacketSize?: number; // default: 65000
/** Time to wait for an onShutdown flush to finish before closing the sender */
flushTimeout?: number; // default: 2000
//The HTTP endpoint for sending spans directly to a collector, i.e. http://jaeger-collector:14268/api/traces
//If setten will override host and port
endpoint?: string;
//Username to send as part of "Basic" authentication to the collector endpoint
username?: string;
//Password to send as part of "Basic" authentication to the collector endpoint
password?: string;
}

// Below require is needed as jaeger-client types does not expose the thrift,
Expand All @@ -41,6 +48,9 @@ export const Utils = require('jaeger-client/dist/src/util').default;
// tslint:disable-next-line:variable-name
export const ThriftUtils = require('jaeger-client/dist/src/thrift').default;

export const HTTPSender = require('jaeger-client/dist/src/reporters/http_sender')
.default;

export type TagValue = string | number | boolean;

export interface Tag {
Expand Down