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

Measure cache hits and misses #169

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

MyDogTom
Copy link
Collaborator

Attach BuildOperationListener to the Gradle build and measure cache hits and misses. This covers both local and remote cache.

How it works
BuildCacheOperationListener listens for two events: ExecuteTaskBuildOperationType (task was executed) and BuildCacheRemoteLoadBuildOperationType (attempt to download remote cache). Summarised information is exposed as ExecutedTasksInfo entity. This information is used by TalaiotPublisherImpl in order to attach cache usage summary to ExecutionReport.Environment and to every task.

Inspired by: BuildCacheConnectionMeasurer from gradle-doctor plugin

Automated tests
I covered major parts with unit tests. Unfortunately, I couldn't come up with a proper e2e test. I think it's very important to have such e2e because this functionality relies on Gradle internal implementation. If you can help me and create an e2e test as a separate PR, that would be nice.

Attach BuildOperationListener to the Gradle and measure cache hits and misses. This covers local and remote cache. BuildCacheOperationListener listen for two events: ExecuteTaskBuildOperationType (task was executed) and BuildCacheRemoteLoadBuildOperationType (attempt to download remote cache). Summarized information is exposed as ExecutedTasksInfo entity. This information is used by TalaiotPublisherImpl in order to attach cache usage summary to ExecutionReport.Environment and to every task.

Inspired by: https://github.com/runningcode/gradle-doctor/blob/321f4d09856aa07ddb2541e205b7684c467d5779/doctor-plugin/src/main/java/com/osacky/doctor/BuildCacheConnectionMeasurer.kt
@cdsap
Copy link
Owner

cdsap commented Apr 29, 2020

what do you need in terms of the E2E?
With testContainers we could have a lot flexibility, even we can create a Remote Cache Instance in case is needed.

@MyDogTom
Copy link
Collaborator Author

what do you need in terms of the E2E?

I want to verify that event BuildCacheRemoteLoadBuildOperationType is emitted by Gradle. Because if at some point they change internals and stop emit it, then functionality won't work anymore. It would be nice to detect it.

I guess that Remote Cache Instance is needed for that.

Copy link
Owner

@cdsap cdsap left a comment

Choose a reason for hiding this comment

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

Great, excellent work.
Just a couple of minor comments.
I created #171 as we discussed in the issue.

import com.cdsap.talaiot.entities.CacheInfo
import com.cdsap.talaiot.metrics.base.ExecutedTasksMetric

class CacheHitMetric : ExecutedTasksMetric<BuildCacheUsageStats>(
Copy link
Owner

Choose a reason for hiding this comment

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

name of the file is TaskMetrics, can we change?

}
}

when (it.remoteCacheInfo) {
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! I have seen cases when there is bad connectivity Gradle disabled the cache mechanism. Is covered in the when +1

/**
* [Metric] that operates on [ExecutedTasksInfo]
*/
abstract class ExecutedTasksMetric<T>(provider: (ExecutedTasksInfo) -> T, assigner: (ExecutionReport, T) -> Unit): Metric<T, ExecutedTasksInfo>(provider, assigner)
Copy link
Owner

Choose a reason for hiding this comment

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

Can we use ExecutedTasksMetric as File name?

@cdsap cdsap merged commit 66b4bf9 into cdsap:master Apr 30, 2020
@MyDogTom MyDogTom deleted the mydogtom/measure-cache-hit-and-miss branch May 4, 2020 22:51
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