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

Add statsd request success function on handleGetAuthKeyIDs #929

Closed
wants to merge 2 commits into from

Conversation

hneiva
Copy link
Contributor

@hneiva hneiva commented Jul 19, 2024

No description provided.

@hneiva hneiva requested review from a team as code owners July 19, 2024 21:20
@hneiva hneiva requested review from jmhodges and removed request for a team July 19, 2024 21:20
main_test.go Outdated
if err != nil {
t.Errorf("Error setting up request.")
}
ag.statsWriteSuccess(req, "test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally, the stats object on ag would be some sort of mock or other thing that we can inspect afterwards. Or at the very least, we need to ensure that it was called appropriately.

Copy link
Contributor

@jmhodges jmhodges left a comment

Choose a reason for hiding this comment

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

Yeah, agreed with Ben's point about the null object.

We'll need a baseline attempts here.

And this would possibly be nice as capturing the status codes returned. Sometimes these metric libraries provide that out of the box (perhaps in a more recent version than we have). Does it? Or do we need to write the Write/WriteHeader wrappers ourselves?

Finally, I don't see an explanation of this change but I'm assuming you only did one HTTP endpoint to see if this is what we wanted, yeah? Def want the rest.

@hneiva
Copy link
Contributor Author

hneiva commented Jul 23, 2024

I was not able to find a good way of mocking autographer.stats for these tests.

Updated the statsWriteSuccess function to return any errors, and added a test case that captures an error.

I'm assuming you only did one HTTP endpoint to see if this is what we wanted, yeah? Def want the rest.

Exactly. Wanted to keep the PR minimal for adding just the functionality, will add other methods later.

@hneiva hneiva requested review from jmhodges and bhearsum July 25, 2024 20:20
@bhearsum
Copy link
Contributor

And this would possibly be nice as capturing the status codes returned. Sometimes these metric libraries provide that out of the box (perhaps in a more recent version than we have). Does it? Or do we need to write the Write/WriteHeader wrappers ourselves?

@hneiva - We can forego any of the test mocking if you'd like, but can we get this part addressed?

@jmhodges
Copy link
Contributor

For my memory: there is a NoOpClient we can use instead of doing nil checks everywhere. https://pkg.go.dev/github.com/datadog/datadog-go/statsd#NoOpClient

@bhearsum
Copy link
Contributor

bhearsum commented Aug 1, 2024

@hneiva - Is review still needed on this iteration? I think it was decided out of band that some changes would be made?

@hneiva
Copy link
Contributor Author

hneiva commented Aug 1, 2024

No need for this anymore since the work Jeff and I did on other PRs

@hneiva hneiva closed this Aug 1, 2024
@hneiva hneiva deleted the hneiva/statsd branch August 1, 2024 21:13
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.

3 participants