feat(sql): log the size of executions when they complete #4660
Conversation
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
I'd wonder if this would be better as a metric vs. part of artifact storage? Not saying "no" as it'd be interesting to track it here... |
With execution id as one of the tags I think cardinality would be too high. Probably config id too. But I could totally see taking those out and making this a metric. |
|
Yeah :) I LIKE high cardinatlity metric data (YAY Honeycomb!) but... I KINDA lean towards removing them & keeping the data as a metric - would have LOVED pipeline size as a metric LONG ago to show exactly how bad this could be AND before/after artifact store support kinda stuff. Not AGAINST it in the DB but I could see this being a VERY compelling metric to track. |
|
Application of course as a label, pipeline name as a label too ideally though that's iffy as that's getting high cardinality route. BUT makes it a LOT easier to track activity by "bad pipeline behavior" AND find pipelines that are heavy hitters. |
| @@ -64,6 +65,15 @@ class CompleteExecutionHandler( | |||
| message.determineFinalStatus(execution) { status -> | |||
| execution.updateStatus(status) | |||
| repository.updateStatus(execution) | |||
| val executionContextSize = execution.getTotalSize() | |||
| if (executionContextSize.isPresent) { | |||
| log.info("completed pipeline execution size: {},{},{},{},{}", | |||
There was a problem hiding this comment.
Something we've learned after living with this for awhile...this isn't necessarily the size of a pipeline execution once spinnaker is totally done with it. As in, things happen to pipeline executions after this. There's logic below to cancel stages of pipelines that didn't succeed, though we've also seen some succeeded pipelines change after this. We're still looking into it.
This log message still seems useful if not as an absolute number, at least as a way to measure change. As in, before/after of the artifact store, or other code changes that influence the size of the execution context.
There was a problem hiding this comment.
I'd absolutely go for the log message!
|
Maybe this is obvious, but the size of execution contexts can change a ton during execution. Like, the completed size could be a small fraction of the max size. Reducing the max size can be a big win even if the completed size stays the same. Measuring this in some more thorough way is, from what I can tell, a non-trivial exercise. |
|
ORIGINAL thought was some metrics on the storage interfaces to track what was written to the storage layer... but haven't dug AS much on this |
|
@jasonmcintosh is this OK to merge? Not sure @xibz has time to give it a look. |
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
to make changes in size over time more observable. This makes it easier to see the impact of features like the artifact store. Not setting the size in RedisExecutionRepository because the extra objectMapper.writeValueAsString is potentially expensive and I'm not sure how many folks are using redis for this.
dbyron-sf
force-pushed
the
log-execution-size
branch
from
7bf56ea
to
5cda6b2
Compare
|
@Mergifyio update |
✅ Branch has been successfully updated |
to make changes in size over time more observable. This makes it easier to see the impact of features like the artifact store.
Not setting the size in RedisExecutionRepository because the extra objectMapper.writeValueAsString is potentially expensive and I'm not sure how many folks are using redis for this.