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

Add Customizable MetricsReporters to the Measure-Builds Gradle Plugin #38

Conversation

takahirom
Copy link
Contributor

@takahirom takahirom commented Mar 18, 2024

Thank you for the great plugin! I find this plugin to be well-designed and modern, making appropriate use of the latest Gradle features. It's a great example of a plugin that leverages Gradle's capabilities effectively.

Overview

This pull request introduces a new feature that allows users to customize the MetricsReporters in the Measure-Builds Gradle plugin. With this change, users can now provide their own implementation of the MetricsReporter interface to handle the reporting of build metrics according to their specific requirements.

Note

Please note that with the removal of the built-in reporting functionality, users will now need to provide their own implementation of MetricsReporter to handle the reporting of build metrics. This change gives users complete control over how and where the metrics are reported.

I understand that I can fork this plugin and maintain my own version with the changes I need. However, I believe that making the plugin customizable and extensible is beneficial for the open-source community. By working together, we can create a better tool that caters to a wider range of use cases and requirements.

@takahirom
Copy link
Contributor Author

@wzieba @malinajirka
Could you take the time to review this PR?

@wzieba
Copy link
Member

wzieba commented Mar 18, 2024

hi @takahirom ! Thank you for the contribution. I agree with your arguments about making this plugin customizable. Initially, we thought it'll be for internal usage only, but that's true it could be nicely extended by introducing custom reporters.

In the following days, I'll review proposed changes 👍

@takahirom takahirom force-pushed the takahirom/add-repoters-to-MeasureBuildsExtension/2024-03-18 branch from 1c8da4b to f126cfa Compare March 19, 2024 12:39
@takahirom
Copy link
Contributor Author

Hi @wzieba
Thank you for considering the introduction of custom reporters!
I have worked on the refactoring and discovered a bug where the users' reports were not properly handled through configuration changes.
I believe the issue is resolved now. I'm referring to the buildScan's buildScanPublished{} extension. However, we're unable to view the buildScan's implementation. It seems that the Action interface is utilized within this extension, which is why I opted for Property.

Copy link
Member

@wzieba wzieba left a comment

Choose a reason for hiding this comment

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

Thank you for your solid work @takahirom on this feature! I'm after the first round of review and I haven't spotted any blocking request changes.

I'll experiment with the implementation more on our repositories to make sure lack of regression. In the meantime, I've run CI checks and some code styling checks are failing. You can fix more of them by running gw :measure-builds:detekt --auto-correct

Comment on lines 45 to 49
suspend fun report(
metricsReport: MetricsReport,
projectName: String,
authToken: String?,
) {
Copy link
Member

Choose a reason for hiding this comment

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

It is tempting to make some Report interface and pass reporter-specific parameters (like projectName or authToken here) as constructor parameters, as you did in 3bb9601 with MetricsReporter.

I like the current approach with buildMetricsPreparedAction more, though, as it's not unnecessarily constraining the implementation. Great job 👍 !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Implementing a configuration cache can be challenging when using instances, and errors are likely to occur.

@takahirom
Copy link
Contributor Author

Hi @wzieba, I've utilized the detekt autocollect task and added a typealias. If you have any feedback or suggestions, please let me know. Feel free to modify my implementation by making changes directly in this PR.

@wzieba
Copy link
Member

wzieba commented Mar 21, 2024

hi @takahirom , thank you for applying the suggestions. I really like this new API and the flexibility that it brings 🙌 .

While experimenting with the new API introduced in this PR though, I've noticed a problem when using it with Groovy. I've prepared an explanation and fix in #43. Please have no hesitation to leave comments and/or suggestions there.

And just a heads-up - I need to perform a few more tests before merging this PR, but I'll be able to do this only early next week. I hope that's ok for you. In any case, if you'd like, you can check a snapshot of the plugin built from the mirror branch (#39) using coordinates like:

plugins {
    id 'com.automattic.android.measure-builds' version '39-1accd072d3d243f9fab57033780d2ab0d53d3692'
}

In the future, I plan to make this plugin available from Gradle Plugin Portal (#42) so it'll be easier.

@wzieba
Copy link
Member

wzieba commented Mar 26, 2024

Thanks again! Looks good with #43 and #44 fixes.

@wzieba wzieba merged commit a38c5e9 into Automattic:main Mar 26, 2024
9 checks passed
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.

None yet

2 participants