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

3528 add monitoring tags #3550

Merged
merged 5 commits into from
Oct 3, 2024
Merged

3528 add monitoring tags #3550

merged 5 commits into from
Oct 3, 2024

Conversation

brostk
Copy link
Contributor

@brostk brostk commented Oct 2, 2024

What was the problem?

Monitoring tags are required by the VA but not present on metrics.

Associated tickets or Slack threads:

How does this fix it?[^1]

MetricLoggerService now includes all required tags with values populated by properties defined in application.yaml.

@brostk brostk requested a review from a team as a code owner October 2, 2024 18:17
@brostk brostk requested review from chengjie8 and dfitchett October 2, 2024 18:17
Copy link
Contributor

github-actions bot commented Oct 2, 2024

Test Results

136 tests  ±0   136 ✅ ±0   19s ⏱️ ±0s
 39 suites ±0     0 💤 ±0 
 39 files   ±0     0 ❌ ±0 

Results for commit c497fa7. ± Comparison against base commit ed95e6e.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Oct 2, 2024

JaCoCo Test Coverage

Overall Project 71.14% -0.87%
Files changed 72.45%

File Coverage
IMetricLoggerService.java 100% 🍏
BipApiService.java 98.87% 🍏
BipRequestErrorHandler.java 88.12% 🍏
MetricLoggerService.java 84.15% -8.23%
InvalidPayloadRejectingFatalExceptionStrategy.java 0% 🍏

Copy link
Contributor

@dfitchett dfitchett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here propagate into the ContentionEventsListener class in the svc-bie-kafka and some of the tags will be duplicated. Can you extend your review to fix that as well?

@dfitchett
Copy link
Contributor

Ah one other thing that is just a thought... what about making the metric prefix also part of this MetricLogger class and imported as a property?

@brostk
Copy link
Contributor Author

brostk commented Oct 3, 2024

The changes here propagate into the ContentionEventsListener class in the svc-bie-kafka and some of the tags will be duplicated. Can you extend your review to fix that as well?

This was your recent addition, right? Any objections to removing the tags here in favor of the common code addition in MetricLoggerService?

Ah one other thing that is just a thought... what about making the metric prefix also part of this MetricLogger class and imported as a property?

Looks plenty do-able, but it'll touch a few interfaces. Be prepared for a larger PR!

Also, the log.info statements when the metrics were successfully submitted should probably be debug not info

I can update these while I'm in the code.

Copy link
Contributor

@dfitchett dfitchett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🚀

@brostk brostk merged commit 231c36a into develop Oct 3, 2024
1 check passed
@brostk brostk deleted the brostk/3528-bip-monitoring-tags branch October 3, 2024 20:25
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.

2 participants