-
Notifications
You must be signed in to change notification settings - Fork 792
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-exporter): allow configuration from env #2099 #2101
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2101 +/- ##
==========================================
+ Coverage 92.76% 94.07% +1.31%
==========================================
Files 137 50 -87
Lines 4904 1721 -3183
Branches 1012 379 -633
==========================================
- Hits 4549 1619 -2930
+ Misses 355 102 -253
|
Seems like quite a few changes are here which are not related to environment |
Because all the defaults was repeated in each package and each platform, i refactored it to centralize in a single place the config for all packages. |
Even though there are conflicts + tests are failing, i would like some reviews to know if we agree on the way of doing it |
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.
I think there are too many changes here that are moved back to base class and they should not be there. Each class of different collector that inherits from base class has its own params that are not needed elsewhere. You have 6 exporters in total. For example headers are - they are not needed in grpc but are needed in in http and browser. I think it will be less errors and changes to just update few places with those few env variables. Trace / span exporter should be interested in 2 variable only, the span exporter for grpc one variable. etc. and the same for metric exporter. Those can be really a small changes.
|
||
export const parsePairKeyValue = (entry: string) => { | ||
const valueProps = entry.split(PROPERTIES_SEPARATOR); | ||
if (valueProps.length <= 0) return; |
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.
Nit / suggestions
if (valueProps.length <= 0) return; | |
if (valueProps.length <= 0) { | |
return; | |
} |
if (!keyPairPart) return; | ||
const keyPair = keyPairPart.split(KEY_PAIR_SEPARATOR); | ||
if (keyPair.length !== 2) return; | ||
const key = decodeURIComponent(keyPair[0].trim()); |
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.
Nit: line space
@@ -69,6 +69,12 @@ export type ENVIRONMENT = { | |||
OTEL_EXPORTER_JAEGER_USER?: string; | |||
OTEL_LOG_LEVEL?: DiagLogLevel; | |||
OTEL_RESOURCE_ATTRIBUTES?: string; | |||
OTEL_EXPORTER_OTLP_ENDPOINT?: string; |
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.
pls keep it in alphabetical order so it is easier to find / add new one
@@ -102,6 +108,12 @@ export const DEFAULT_ENVIRONMENT: Required<ENVIRONMENT> = { | |||
OTEL_SPAN_ATTRIBUTE_COUNT_LIMIT: 1000, | |||
OTEL_SPAN_EVENT_COUNT_LIMIT: 1000, | |||
OTEL_SPAN_LINK_COUNT_LIMIT: 1000, | |||
OTEL_EXPORTER_OTLP_ENDPOINT: '', |
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.
pls keep it in alphabetical order so it is easier to find / add new one
*/ | ||
import { Baggage, baggageEntryMetadataFromString } from '@opentelemetry/api'; | ||
|
||
export const KEY_PAIR_SEPARATOR = '='; |
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.
if you split this already what if you keep all const in separate file ? constants.ts
for example ?
and then later in code instead of
baggageUtils.BAGGAGE_HEADER
you will have for example
baggageConst.BAGGAGE_HEADER
// or
constants.BAGGAGE_HEADER
collector.serviceClient = new packageObject.opentelemetry.proto.collector.trace.v1.TraceService( | ||
serverAddress, | ||
credentials | ||
); | ||
} else { | ||
} else if (collector.getExporterType() === 'metric') { |
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.
Hmm you have removed the ServiceClientType
and then in both metrics and spans added a trace
. I think having a predefined enum / const like it was ServiceClientType
is pretty straightforward ?
@@ -41,15 +40,15 @@ export function onInit<ExportItem, ServiceRequest>( | |||
root.resolvePath = function (origin, target) { | |||
return `${dir}/${target}`; | |||
}; | |||
if (collector.getServiceClientType() === ServiceClientType.SPANS) { | |||
if (collector.getExporterType() === 'trace') { |
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 removing the ServiceClientType
and then using hardcoded value ?
this.url = this.getDefaultUrl(config); | ||
this.serviceName = this._getDefaultServiceName(config); | ||
this.url = this._getUrl(config); | ||
this.headers = this._getHeaders(config); |
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.
headers are not needed for grpc so they should not be here ?
} | ||
} | ||
|
||
private _getDefaultServiceName(config: T): string { |
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.
keeping this in base will get extra overhead for each instance. Like for example metric exporter doesn't need to know the name of trace exporter and opposite. Plus each instance will have extra function for just getting it's name. Whereas you can keep the final name in the specific class.
} | ||
} | ||
|
||
private _getHeaders(config: T): Record<string, string> { |
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.
the same with headers, base class is used by all 3 protocols, grpc doesn't need this at all. That's why I think the headers should not be a part of base class
@@ -132,14 +143,106 @@ export abstract class CollectorExporterBase< | |||
return this._shuttingDownPromise; | |||
} | |||
|
|||
private _getUrl(config: T): string { |
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.
The cyclomatic complexity of this function is really high 16-17 if I counted correctly.
The whole purpose of splitting the classes was to put the final information in the final class like for example name, default url etc.. Imagine now you have 50 different exporters and you want to create a default url for them. I think doing this in base class becomes very complicated as it will produce an enormous complex function, not even mentioning about all possible tests cases.
Closing this PR since addressing changes is basically a new PR, see #2117 |
I centralized default configuration inside the main package
exporter-collector
, each variant of the exporter (for each signal/protocol) is defined with abstract functions.I'm not sure if everything is spec compliant, there actually a lot of possible mix: https:/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md
I've also changed the default port for HTTP which seems to be
4317
for all protocols: https:/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/otlp.md#otlphttp-default-portSince the headers configuration is using the same correlation format that the baggage, i modified the core package to export utils to parse them (i expect more configurations will use this later on)
Fixes #2099