Skip to content

Conversation

@kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Dec 1, 2024

Description

Omit the 'curl' library if DD_TRACE_TRANSPORT is set to "none"

Motivation

There is clearly the intent to be able to not include a default transport, by setting DD_TRACE_TRANSPORT=none.
However, two things were missing:

  1. To ignore the bundled curl library from the build
  2. to replace the default_http_client_curl.cpp with default_http_client_null.cpp

Integrations that provide their own transport may not want to build and link a whole unused library, along with any runtime library dependencies it makes.

Additional Notes

@kristjanvalur kristjanvalur requested a review from a team as a code owner December 1, 2024 12:59
@kristjanvalur kristjanvalur requested review from zacharycmontoya and removed request for a team December 1, 2024 12:59
@dmehala
Copy link
Collaborator

dmehala commented Dec 1, 2024

Fantastic @kristjanvalur.
This is indeed the intended behaviour. I appreciate your contributions.

Copy link
Collaborator

@dmehala dmehala left a comment

Choose a reason for hiding this comment

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

LGTM!
If I recall correctly, when no HTTP client is provided, the library defaults to default_http_client. This could potentially lead to a crash since the library assumes default_http_client is always a valid instance. While it functions, some refactoring would be beneficial to help users avoid unintended issues.

I think the tradeoff is ok, as long as it help you progress on your plugin :)

Thank you!

@kristjanvalur
Copy link
Contributor Author

I could add a check in the config builder for a nullptr, but since this is an integration issue and not a configuration issue, in my view assuming a non-null ptr (and thus a correctly written integration) is ok.

@dmehala dmehala merged commit febab81 into DataDog:main Dec 2, 2024
19 checks passed
@kristjanvalur kristjanvalur deleted the kristjan/transport branch December 2, 2024 16:28
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.

2 participants