-
Notifications
You must be signed in to change notification settings - Fork 3k
Build metrics: make it possible to collect produced items #51396
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
base: main
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
40d5fa5 to
e2b50ad
Compare
This comment has been minimized.
This comment has been minimized.
phillip-kruger
left a comment
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.
LGTM
| this.enabled = Boolean.getBoolean("quarkus.builder.metrics.enabled") | ||
| // This system property is deprecated and will be removed | ||
| || Boolean.getBoolean("quarkus.debug.dump-build-metrics"); |
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.
Tiny nitpick: I personally find Boolean.getBoolean, which resolves from a system property, very confusing...
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.
Mee too ;-). It should have been Boolean.getBooleanSystemProperty() or something like this. OTOH it's a one-liner and it somehow fits in this part of the code I think 🤷.
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.
Sure yeah, as I said it's minor nitpick :)
gsmet
left a comment
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.
The or in do not collect any metrics unless quarkus.builder.metrics.enabled=true or in the dev mode bugs me a bit.
It's always enabled in dev mode? Could we make sure the cost is not too high if so because I remember similar infrastructure which had a relatively high cost.
Another alternative would be to only enable it if quarkus.builder.metrics.enabled=true and have the Dev UI page mention it if not enabled.
I made it Request for change so we clarify this, I'm fine with this approach if the impact is not too high.
Yes, it is and it always was (since 2.11.0.CR1 where we introduced the build metrics in #25786).
It should not be a problem for most apps. For example, for
|
- if quarkus.builder.metrics.extended-capture=true - show the produced build items in the Dev UI - do not collect any metrics unless quarkus.builder.metrics.enabled=true - in the dev mode, collect the metrics by default unless quarkus.builder.metrics.enabled=false - deprecate quarkus.debug.dump-build-metrics
e2b50ad to
becdd72
Compare
|
@gsmet I wasn't able to get some consistent results on my machine. The augmentation time varies a lot, esp. for apps with a large number of extensions. However, I made two more changes:
|
Status for workflow
|
This could help analyzing the problems like the one mentioned in #51120.