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(stackdriver): read INT64 metrics correctly #933

Merged
merged 8 commits into from
Jan 10, 2024

Conversation

tomklino
Copy link
Contributor

The StackDriver metric service was hardcoded to read only doubleValue of metrics. However, some metrics are reported from StackDriver as INT64. When the service tries to read them as a doubleValue, they get back a 0, regardless of the true value. Added a test for the type of metric reported, and logic to read and map accordingly. In case of an unexpected value, preserved the previous behavior (reading as doubleValue)

@tomklino tomklino marked this pull request as draft March 11, 2023 19:10
@tomklino tomklino marked this pull request as ready for review December 25, 2023 22:15
@tomklino
Copy link
Contributor Author

Hi @dbyron-sf !

It's been a while :-)

I got some help and put in the time to write the test (I also wrote another one while I was at it). I ran the test and it's green when the fix is applied and red when running with the original code.

It's ready for review, please take a look and tell me what you think.

Thank you

@InjectMocks StackdriverMetricsService stackdriverMetricsService;

@Test
public void readsInt64Metrics() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails without the code change above, which is great!

@dbyron-sf
Copy link
Contributor

Hi @dbyron-sf !

It's been a while :-)

I got some help and put in the time to write the test (I also wrote another one while I was at it). I ran the test and it's green when the fix is applied and red when running with the original code.

It's ready for review, please take a look and tell me what you think.

Thank you

Thanks so much for sticking with this!

@tomklino
Copy link
Contributor Author

tomklino commented Jan 6, 2024

Of course, it's my pleasure!

@dbyron-sf dbyron-sf added the ready to merge Reviewed and ready for merge label Jan 8, 2024
@mergify mergify bot added the auto merged label Jan 8, 2024
@tomklino
Copy link
Contributor Author

Hi David, thank you for the review and the help.

Is there anything else that needs to be done from my side? I see there's a failed check but I don't know if that's anything I can fix, seems like a permission issue

@mergify mergify bot merged commit a560c2f into spinnaker:master Jan 10, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants