Simplify interpretation of produced metrics #23
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Add pre-computed phase durations * Introduce JSON specs for metric output
Izzette
commented
Mar 30, 2024
Izzette
force-pushed
the
change-metric-spec
branch
from
28ba3e6
to
d3d39b6
Compare
nicmarti
reviewed
Apr 22, 2024
| @@ -46,32 +50,68 @@ func (cs containerStatistic) logger() zerolog.Logger { | |||
| Logger() | |||
| } | |||
|
|
|||
| func (cs containerStatistic) appendInitFields(event *zerolog.Event) { | |||
| if !cs.runningTimestamp.IsZero() && cs.previous != nil && !cs.previous.readyTimestamp.IsZero() { | |||
There was a problem hiding this comment.
cs.runningTimeStamp is always != nil ?
There was a problem hiding this comment.
yeah, it's a time.Time not a pointer to time.Time so it's the time.Time zero value until set, which is—I believe—the unix epoch.
There was a problem hiding this comment.
So yes technically this code won't work before 1970
There was a problem hiding this comment.
(it was a joke, happy to see that someone read my comment 😀 )
Izzette
commented
Apr 22, 2024
Comment on lines
+90
to
+91
| // Instead, readiness represents the time an init container has excited successfully,allowing the following containers | ||
| // to proceed. |
There was a problem hiding this comment.
Suggested change
| // Instead, readiness represents the time an init container has excited successfully,allowing the following containers | |
| // to proceed. | |
| // Instead, readiness represents the time until an init container has exited successfully, allowing the following | |
| // containers to proceed. |
thylong
reviewed
Jun 10, 2024
| writer http.ResponseWriter, | ||
| req *http.Request, | ||
| ) { | ||
| startTime := time.Now() | ||
| logger := c.logger() | ||
|
|
||
| responseLogger := &responseLogger{responseWriter: writer} | ||
| responseLogger := &httpResponseLogger{responseWriter: writer} |
There was a problem hiding this comment.
In the context of ServeHTTP method, an http prefix can be skipped IMHO
Izzette
commented
Jun 10, 2024
Izzette
force-pushed
the
change-metric-spec
branch
from
17b189d
to
d5b7db4
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO:
partial: true/falseattribute and emit this with true only once when the pod becomes ready or is deleted before it becomes ready. This will allow aggregations based on the data to avoid having duplicate partial datapoints resulting in skewed distributions depending on how many events were received before the "final state" was achieved. This may be better to do in a different PR.