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
fix(metrics): only add required artifacts to application #6644
Conversation
cf8c167
to
306103a
Compare
dd55910
to
1fca28f
Compare
extensions/metrics/src/__tests__/acceptance/metrics-push.acceptance.ts
Outdated
Show resolved
Hide resolved
...DEFAULT_METRICS_OPTIONS, | ||
...metricsConfig, | ||
}; | ||
if (options.defaultMetrics && !options.defaultMetrics.disabled) { |
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.
It can be simplified as:
if (!options.defaultMetrics?.disabled) {
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.
its actually not the same if options.pushGateway
is undefined
it will be true whereas options.pushGateway && !options.pushGateway.disabled
is false becasue !undefined === 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.
if (options.defaultMetrics?.disabled !== true)
is probably the simplest.
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 does not work in case of the pushGateway
which is undefined and we dont want to add the MetricsPushObserver
in that case
@nflaig Please rebase against latest master to resolve conflicts. |
- only add `MetricsPushObserver` to application if configured - merge custom config with default options instead of overwriting Signed-off-by: nflaig <nflaig@protonmail.com>
1fca28f
to
c3b7d5c
Compare
@raymondfeng I removed some changes from the PR and only added the fix now to not add the The other consideration was to add |
This PR resolves three issues of the component:
MetricsPushObserver
even if it is not configuredChecklist
npm test
passes on your machine