feat(exporters)!: rewrite exporter config logic #4971
Conversation
pichlermarc
force-pushed
the
feat/node-exporter-config-rewrite
branch
2 times, most recently
from
2b185dc
to
af05548
Compare
pichlermarc
force-pushed
the
feat/node-exporter-config-rewrite
branch
2 times, most recently
from
9bfaad5
to
b93d9db
Compare
pichlermarc
force-pushed
the
feat/node-exporter-config-rewrite
branch
from
11b5f20
to
3c1756f
Compare
pichlermarc
changed the title
| import { getSharedConfigurationFromEnvironment } from './shared-env-configuration'; | ||
| import { OtlpHttpConfiguration } from './otlp-http-configuration'; | ||
|
|
||
| function getHeadersFromEnv(signalIdentifier: string) { |
There was a problem hiding this comment.
nit: do you think signals are going to grow much in the future that we need to have the identifier as a string type?
There was a problem hiding this comment.
Hmm, I don't think they will grow much. I mostly just chose string as it was the easiest choice to work with and it does not require us to export a new type that could be difficult to extend later.
Do you think we should have a SignalIdentifier type? 🤔
There was a problem hiding this comment.
one might think is a commodity but with the type the editor helps you to avoid passing a string which does not refer to any Signal. Is not wrong to not have a type here. I'll leave it to you
There was a problem hiding this comment.
I think I will leave it as-is - maybe I'm traumatized by making unintentional breaking changes in TypeScript libraries, but I feel like whatever type I can come up with won't be extendable in a safe way later on. 😕
The fact that libraries have to be type-compatible with other versions of themselves is kind of a mind-bender that did not exist in other languages I came from originally. This API will mostly be internal, so I'll opt to leave it like this for now. We can re-visit this choice in a later iteration - I'm planning to have a final round of API review when the whole exporter restructuring is over to work out smaller things like this. 🙂
| if (!url.endsWith('/')) { | ||
| url = url + '/'; | ||
| } | ||
| return url + path; |
There was a problem hiding this comment.
The function is not checking that
urldoes not already contain a path- the result string is a valid URL, path could contain invalid chars
So the following call is possible
appendResourcePathToUrl('http://foo.com/bar/test.txt', 'v1/logs')
// returns 'http://foo.com/bar/test.txt/v1/logs'I guess we should let the users define the URL they want but I think we should diag.warn or diag.debug the result for debug purposes (please ignore this comment if the diag. is set in a file below, I haven't reached them yet)
There was a problem hiding this comment.
Good point about checking that the final URL is valid. 👍 I added additional checks and logs to that function. I settled on warn as it's very likely that it'll not be able to continue with an invalid URL. 5d0aae1
It's actually intended to append if it does not contain a path already - this is specified behavior for the OTEL_EXPORTER_OTLP_ENDPOINT env var where this function is used. See https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md#endpoint-urls-for-otlphttp
For signal-specific ones like OTEL_EXPORTER_OTLP_ENDPOINT no path is actually appended - just the root path if none is there already that's the function above this one 🙂
| ); | ||
| assert.deepStrictEqual(config.headers, { | ||
| foo: 'foo-user-provided', | ||
| baz: 'null', |
There was a problem hiding this comment.
bar: undefined does not appear in the result but baz: null does and it's stringified. Any reason for that?
There was a problem hiding this comment.
This is mostly to keep the same behavior of the current implementation, which also handles it this way 🙂
undefined
bar: undefined is dropped as calls with http module will throw when there's an undefined header value:
> const http = require('http');
undefined
> http.get({host: 'example.com', headers: {'foo': undefined}})
Uncaught:
TypeError [ERR_HTTP_INVALID_HEADER_VALUE]: Invalid value "undefined" for header "foo"
at __node_internal_captureLargerStackTrace (node:internal/errors:496:5)
at new NodeError (node:internal/errors:405:5)
at __node_internal_ (node:_http_outgoing:624:11)
at ClientRequest.setHeader (node:_http_outgoing:651:3)
at new ClientRequest (node:_http_client:291:14)
at request (node:http:100:10)
at Object.get (node:http:111:15) {
code: 'ERR_HTTP_INVALID_HEADER_VALUE'
}
We've had users in the past that have ended up with undefined header values as they set a header to process.env.MY_HEADER_ENV_VAR, did not set that env var and then ended up with an exporter that would constantly fail.
null
null is stringified as to ensure it matches the type for Record<string, string> for the headers. Plus it gets stringified to null anyway when it's sent via with the http module so we make it happen earlier to make the type a bit more simple to work with down the chain as we don't have to worry about null values. It's also the current behavior of the exporters, so I tried not to change it.
You can try sending it to https://webhook.site to see that it ends up the same:
> const https = require('https');
undefined
> https.get({host: 'webhook.site', headers: {'foo': null}, path: '/<your-webhook-site-path>'})
> https.get({host: 'webhook.site', headers: {'foo': 'null'}, path: '/<your-webhook-site-path>'})
Which problem is this PR solving?
Config code for exporters (user-provided, defaults, and env config) is currently hard to test individually and is spread/duplicated all over the different signal exporters.
This PR moves the configuration as much as possible into separate files/functions and splits them up into:
Since most config is now moved to base-packages we can remove the in-depth tests from the specific packages. This reduces duplication and makes further changes easier, as they will touch "fewer" files. Since now the configuration-code is not part of the individual exporter-implementations, collapsing the base exporters into one class becomes simpler as we have to worry less about the inheritance structure (abstract methods were removed in this PR as they were config-related).
In addition to these improvements, this PR also fixes #3748, #3747 which was a side-effect of the previous way of implementing env var config.
Breaking changes:
getDefaultUrlwas intended for internal use has been removed from all exportersgetUrlFromConfigwas intended for internal use and has been removed from all exportershostnamewas intended for internal use and has been removed from all exportersurlwas intended for internal use and has been removed from all exporterstimeoutMilliswas intended for internal use and has been removed from all exportersonInitwas intended for internal use and has been removed from all exportersparseHeadersis now not exported anymoreappendResourcePathToUrlis now not exported anymoreappendResourcePathToUrlIfNeededis now not exported anymoreconfigureExporterTimeoutis now not exported anymoreinvalidTimeoutis now not exported anymoreFollow-up steps:
OTEL_EXPORTER_OTLP_TEMPORALITY_PREFERENCE, this will be done in a follow-up.OTLPExporterNodeBase,OTLPExporterBrowserBase,OTLPGrpcExporterNodeBaseinto one shared base, that favors composition over inheritance. This can be done by:OTLPExporterDelegate(draft) based on the "new" config, this replaces theOTLPExporterNodeBase,OTLPExporterBrowserBase,OTLPGrpcExporterNodeBaseclasses which right now do essentially the same, except for the constructor which will be replaced by the factory function.@opentelemetry/otlp-transformer, testing that the output is the correct format should suffice)Fixes #3748
Fixes #3747
Type of change
How Has This Been Tested?