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 configurable error status codes to httprb and httpclient #2576

Merged
merged 5 commits into from
Jan 25, 2023

Conversation

caramcc
Copy link
Contributor

@caramcc caramcc commented Jan 20, 2023

What does this PR do?
Adds the ability to configure which HTTP status codes are considered errors for http.rb and httpclient as a follow-up to #2501 which added this option to Net/HTTP.

Motivation
The applications I'm instrumenting make a lot of API calls to external services using a few different HTTP libraries, including http.rb and httpclient. Since the services are user-configured, we expect a relatively large volume of these requests to fail with 4xx or 5xx status codes. As a result, we end up ingesting a lot of traces due to ingestion_reason:error that we don't consider to be an error and don't otherwise care about.

Adding the ability to configure which HTTP status codes are considered errors will allow us to reduce our ingestion rates without entirely ignoring all http.rb or httpclient spans that return a 4xx or 5xx status code.

Additional Notes
Let me know if you'd prefer I split the change for httpclient into a separate PR.

This uses the same shared specs and configuration pattern from my previous PR (#2501) which added this feature to Net/HTTP.

How to test the change?
I tested the changes with bundle exec appraisal ruby-3.0.4-contrib rake spec:httprb and bundle exec appraisal ruby-3.0.4-contrib rake spec:httpclient

@caramcc caramcc requested a review from a team January 20, 2023 22:30
@github-actions github-actions bot added integrations Involves tracing integrations tracing labels Jan 20, 2023
Copy link
Contributor

@TonyCTHsu TonyCTHsu left a comment

Choose a reason for hiding this comment

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

👋 @caramcc, Thanks for the contribution, the PR looks 👍 .

We usually want environment variables for each contrib, by prefixing DD_TRACE_#{contrib}_..... We would appreciated some specs for picking up the environment variable to configure error codes.

Last time I checked, your PR contains the config file but I was not quite sure that why CircleCI refused to run your PR. Perhaps you could rebase on latest master?
Screenshot 2023-01-23 at 12 49 23

@@ -11,6 +11,7 @@ module Ext
ENV_SERVICE_NAME = 'DD_TRACE_HTTPRB_SERVICE_NAME'.freeze
ENV_ANALYTICS_ENABLED = 'DD_TRACE_HTTPRB_ANALYTICS_ENABLED'.freeze
ENV_ANALYTICS_SAMPLE_RATE = 'DD_TRACE_EHTTPRB_ANALYTICS_SAMPLE_RATE'.freeze
ENV_ERROR_STATUS_CODES = 'DD_TRACE_HTTP_ERROR_STATUS_CODES'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

The value should be 'DD_TRACE_HTTPRB_ERROR_STATUS_CODES'

@@ -11,6 +11,7 @@ module Ext
ENV_SERVICE_NAME = 'DD_TRACE_HTTPCLIENT_SERVICE_NAME'.freeze
ENV_ANALYTICS_ENABLED = 'DD_TRACE_HTTPCLIENT_ANALYTICS_ENABLED'.freeze
ENV_ANALYTICS_SAMPLE_RATE = 'DD_TRACE_HTTPCLIENT_ANALYTICS_SAMPLE_RATE'.freeze
ENV_ERROR_STATUS_CODES = 'DD_TRACE_HTTP_ERROR_STATUS_CODES'.freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

The value should be 'DD_TRACE_HTTPCLIENT_ERROR_STATUS_CODES'

@caramcc
Copy link
Contributor Author

caramcc commented Jan 23, 2023

Last time I checked, your PR contains the config file but I was not quite sure that why CircleCI refused to run your PR. Perhaps you could rebase on latest master?

Just made the requested changes and rebased on latest master, but rebasing doesn't seem to have helped with this unfortunately.

I noticed this issue with my previous PR as well, I'd assumed that those OAuth permissions are set to prevent people from outside the Datadog organization from running CI/CD for security purposes, could that be what's going on here?

@TonyCTHsu
Copy link
Contributor

I noticed this issue with my previous PR as well, I'd assumed that those OAuth permissions are set to prevent people from outside the Datadog organization from running CI/CD for security purposes, could that be what's going on here?

There was CircleCI security breach recently and a lots of security measurements were taken. There were other external contributor successfully submitted PR and have CircleCI run for them. Could you follow this guide to revoke your CircleCI credentials in GitHub and then re-authenticate?

If the issue still persist, I could force push your branch to trigger the build.

@ivoanjo
Copy link
Member

ivoanjo commented Jan 24, 2023

On top of what Tony said, after you follow the CircleCI steps, you'll probably need to do another push to get CircleCI to retry. (It otherwise seems to cache the failure state)

Thanks for the patience with CI things :)

@TonyCTHsu TonyCTHsu merged commit fe932e9 into DataDog:master Jan 25, 2023
@TonyCTHsu TonyCTHsu added this to the 1.9.0 milestone Jan 25, 2023
@caramcc caramcc deleted the cmm-httprb-httpclient branch January 25, 2023 22:00
@TonyCTHsu TonyCTHsu mentioned this pull request Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrations Involves tracing integrations tracing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants