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.

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

Merged
merged 1 commit into from
Oct 28, 2020

Conversation

nflaig
Copy link
Member

@nflaig nflaig commented Oct 25, 2020

This PR resolves three issues of the component:

  1. The component adds MetricsPushObserver even if it is not configured
  2. Providing custom config was overwriting whole default config instead of merging

Checklist

  • DCO (Developer Certificate of Origin) signed in all commits
  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated

@nflaig nflaig force-pushed the fix/metrics_component branch 2 times, most recently from cf8c167 to 306103a Compare October 25, 2020 15:06
@nflaig nflaig changed the title fix(metrics): only add required artifacts to application extension/metrics: improve configuration and only add required artifacts to application Oct 25, 2020
...DEFAULT_METRICS_OPTIONS,
...metricsConfig,
};
if (options.defaultMetrics && !options.defaultMetrics.disabled) {
Copy link
Contributor

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) {

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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

@raymondfeng
Copy link
Contributor

@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>
@nflaig nflaig changed the title extension/metrics: improve configuration and only add required artifacts to application fix(metrics): only add required artifacts to application Oct 27, 2020
@nflaig
Copy link
Member Author

nflaig commented Oct 27, 2020

@raymondfeng I removed some changes from the PR and only added the fix now to not add the MetricsPushObserver if pushGateway is not configured and the fact that the config was overwritten instead of merged.

The other consideration was to add options.methodInvocations to enable/disable those or as already discussed in the health component, it could be considered to add a options.disabled to completely disable the component although this could also be achieved by wrapping app.component into an if-statement

@raymondfeng raymondfeng merged commit 27f36b0 into loopbackio:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants