-
Notifications
You must be signed in to change notification settings - Fork 828
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(otlp-exporter-base): internally accept a http header provider function only #5179
Changes from 4 commits
b061548
85dc911
a33cc39
a65357f
909666f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,32 +27,34 @@ import type * as https from 'https'; | |
|
||
export interface OtlpHttpConfiguration extends OtlpSharedConfiguration { | ||
url: string; | ||
headers: Record<string, string>; | ||
headers: () => Record<string, string>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @pichlermarc, I have done a similar pr about that for string or function support #5118 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @bozzelliandrea - I'd prefer having only one option internally (as my PR here proposes) to avoid the code becoming too complex. By only dealing with provider functions internally we can handle both static and dynamic use-cases via the same code-path. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @pichlermarc, do you have any idea when the team will release the version with this commit? thanks |
||
agentOptions: http.AgentOptions | https.AgentOptions; | ||
} | ||
|
||
function mergeHeaders( | ||
userProvidedHeaders: Record<string, string> | undefined | null, | ||
fallbackHeaders: Record<string, string> | undefined | null, | ||
defaultHeaders: Record<string, string> | ||
): Record<string, string> { | ||
userProvidedHeaders: (() => Record<string, string>) | undefined | null, | ||
fallbackHeaders: (() => Record<string, string>) | undefined | null, | ||
defaultHeaders: () => Record<string, string> | ||
): () => Record<string, string> { | ||
const requiredHeaders = { | ||
...defaultHeaders, | ||
...defaultHeaders(), | ||
}; | ||
const headers = {}; | ||
|
||
// add fallback ones first | ||
if (fallbackHeaders != null) { | ||
Object.assign(headers, fallbackHeaders); | ||
} | ||
return () => { | ||
// add fallback ones first | ||
if (fallbackHeaders != null) { | ||
Object.assign(headers, fallbackHeaders()); | ||
} | ||
|
||
// override with user-provided ones | ||
if (userProvidedHeaders != null) { | ||
Object.assign(headers, userProvidedHeaders); | ||
} | ||
// override with user-provided ones | ||
if (userProvidedHeaders != null) { | ||
Object.assign(headers, userProvidedHeaders()); | ||
} | ||
|
||
// override required ones. | ||
return Object.assign(headers, requiredHeaders); | ||
// override required ones. | ||
return Object.assign(headers, requiredHeaders); | ||
}; | ||
} | ||
|
||
function validateUserProvidedUrl(url: string | undefined): string | undefined { | ||
|
@@ -107,7 +109,7 @@ export function getHttpConfigurationDefaults( | |
): OtlpHttpConfiguration { | ||
return { | ||
...getSharedConfigurationDefaults(), | ||
headers: requiredHeaders, | ||
headers: () => requiredHeaders, | ||
url: 'http://localhost:4318/' + signalResourcePath, | ||
agentOptions: { keepAlive: true }, | ||
}; | ||
|
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.
Note for reviewers: this was in the wrong spot earlier, so moved it up here.