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

Fix up endTimestamp to be Monarch compliant #879

Merged
merged 6 commits into from
Aug 17, 2022

Conversation

pmarkowsky
Copy link
Contributor

This PR fixes an issue with our Monarch JSON that causes some metrics to be wonky.

Specifically our endTimestamp field needs to be the time of the export not the last_updated time.

@pmarkowsky pmarkowsky added the metrics Code / work related to Santa observability / monitoring label Aug 16, 2022
@pmarkowsky pmarkowsky self-assigned this Aug 16, 2022
@pmarkowsky pmarkowsky added this to the 2022.8 Release milestone Aug 16, 2022
@mlw
Copy link
Contributor

mlw commented Aug 16, 2022

How I'm reading this - it appears like this will make the last_updated values reported by SNTCommandMetrics not match what is sent to the metrics server - is that desireable?

@pmarkowsky
Copy link
Contributor Author

pmarkowsky commented Aug 16, 2022

@mlw Short answer is yes. Monarch requires the time series to be reported constantly so as long as we have the value we have to record a new endTimestamp, not the last time it was actually updated.

If we want last_updated to be the last time the metric changed then we need to do this differently if we just want everything to be the last time we exported then we can simplify the SNTMetricSet to just always add the timestamp on export.

@pmarkowsky pmarkowsky merged commit fd23a5c into google:main Aug 17, 2022
@pmarkowsky pmarkowsky deleted the markowsky/fix-metric-timestamps branch August 17, 2022 02:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
metrics Code / work related to Santa observability / monitoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants