Skip to content

Follow ups for micrometer metrics PFP-1613 #14694

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 9, 2025
Merged

Follow ups for micrometer metrics PFP-1613 #14694

merged 1 commit into from
Sep 9, 2025
+193 −398

Conversation

abedatahub
Copy link
Collaborator

No description provided.

@abedatahub abedatahub self-assigned this Sep 5, 2025
@github-actions github-actions bot added the community-contribution PR or Issue raised by member(s) of DataHub Community label Sep 5, 2025
@datahub-cyborg datahub-cyborg bot added the needs-review Label for PRs that need review from a maintainer. label Sep 5, 2025
@codecov Codecov
Copy link

codecov bot commented Sep 5, 2025

Bundle Report

Bundle size has no change ✅

@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from f01b4d8 to 575280b Compare September 5, 2025 23:05
@codecov Codecov
Copy link

codecov bot commented Sep 5, 2025 •
edited
Loading

Codecov Report

❌ Patch coverage is 95.74468% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
.../elastic/ElasticSearchTimeseriesAspectService.java 0.00% 1 Missing ⚠️
...s/factory/kafka/trace/KafkaTraceReaderFactory.java 0.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from 575280b to 29b8c69 Compare September 6, 2025 23:52
abedatahub
abedatahub commented Sep 6, 2025
View reviewed changes
...bs/mae-consumer/src/main/java/com/linkedin/metadata/kafka/listener/mcl/MCLKafkaListener.java
.timer(MetricUtils.DATAHUB_REQUEST_HOOK_QUEUE_TIME, "hook", hookName)
.record(Duration.ofMillis(queueTimeMs));
});
.timer(MetricUtils.DATAHUB_REQUEST_HOOK_QUEUE_TIME, "hook", hookName)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We should move away from invoking these registry APIs on the hot path. They result in warning logs and the implementation has a synchronization bug in this method

Copy link
Collaborator Author

@abedatahub abedatahub Sep 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

(I'll write that best-practices guide for micrometer / prometheus)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI, best-practices here in this PR: #14711

abedatahub
abedatahub commented Sep 7, 2025
View reviewed changes
.../mae-consumer/src/test/java/com/linkedin/metadata/kafka/DataHubUsageEventsProcessorTest.java
@@ -504,89 +499,13 @@ public void testMicrometerMetricsWithFailedTransformation() {
verify(elasticsearchConnector, never()).feedElasticEvent(any());
}

@Test
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Most of these tests look like they were AI generated. A lot of them verify trivial functionality or verify too strictly. IMO, they have very little value in terms of their ability to catch regressions, and make refactorings such as this PR quite a bit harder.

@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from 29b8c69 to bbcc495 Compare September 7, 2025 00:10
@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch 2 times, most recently from c28fcdf to a545a2f Compare September 7, 2025 00:34
@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from a545a2f to 733707b Compare September 8, 2025 05:01
@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from 733707b to eca800e Compare September 8, 2025 16:11
@alwaysmeticulous alwaysmeticulous
Copy link

alwaysmeticulous bot commented Sep 8, 2025 •
edited
Loading

🔴 Meticulous spotted visual differences in 20 of 1380 screens tested: view and approve differences detected.

Meticulous evaluated ~8 hours of user flows against your PR.

Last updated for commit eca800e. This comment will update as new commits are pushed.

@abedatahub
refactor(metrics): Make MetricUtils.registry non-nullable
7e6d5be 
- Make MetricUtils.registry non-nullable with default no-op implementation
- This allows us to remove boilerplate for handling the null case
- Rename request context metric names to follow convention
@abedatahub abedatahub force-pushed the as--add-micrometer-metrics-PFP-1613 branch from eca800e to 7e6d5be Compare September 8, 2025 17:43
@abedatahub abedatahub changed the title Follow ups for micrometer metrics PFT-1613 Follow ups for micrometer metrics PFP-1613 Sep 8, 2025
@datahub-cyborg datahub-cyborg bot removed the needs-review Label for PRs that need review from a maintainer. label Sep 9, 2025
@datahub-cyborg datahub-cyborg bot added the merge-pending-ci A PR that has passed review and should be merged once CI is green. label Sep 9, 2025
@abedatahub abedatahub merged commit 6ec6f01 into master Sep 9, 2025
43 checks passed
@abedatahub abedatahub deleted the as--add-micrometer-metrics-PFP-1613 branch September 9, 2025 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-leifker david-leifker david-leifker approved these changes

Assignees

@abedatahub abedatahub

Labels
community-contribution PR or Issue raised by member(s) of DataHub Community merge-pending-ci A PR that has passed review and should be merged once CI is green.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@abedatahub @david-leifker