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

Rename env variables to follow DataDog profiling naming conventions #17

Merged
merged 10 commits into from
Nov 5, 2024

Conversation

nsavoire
Copy link
Collaborator

@nsavoire nsavoire commented Sep 27, 2024

What does this PR do?

Rename env variables used for configuration to be consistent with other Datadog profilers (eg DD_SERVICE, DD_PROFILING_UPLOAD_PERIOD).
Also add a prefixed version of these variables (eg DD_HOST_SERVICE, DD_HOST_PROFILING_UPLOAD_PERIOD) that takes precedence over the non-prefixed version for cases where host profiler might be running along side another runtime profiler and a different configuration is needed for the host profiler.

@nsavoire nsavoire requested a review from a team as a code owner September 27, 2024 02:43
@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch from e5bb572 to 84e28d3 Compare September 27, 2024 02:52
cli_flags.go Outdated Show resolved Hide resolved
cli_flags.go Outdated Show resolved Hide resolved
@nsavoire nsavoire force-pushed the nsavoire/opensource branch 3 times, most recently from 03bc08a to 5d65481 Compare September 27, 2024 10:14
Base automatically changed from nsavoire/opensource to main September 27, 2024 13:51
An error occurred while trying to automatically change base from nsavoire/opensource to main September 27, 2024 13:51
@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch 2 times, most recently from cec9b46 to 4e14904 Compare October 1, 2024 00:08
cli_flags.go Outdated Show resolved Hide resolved
reporter/datadog_upload.go Fixed Show fixed Hide fixed
@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch 3 times, most recently from a708aa0 to 2d669ba Compare October 7, 2024 07:53
README.md Show resolved Hide resolved
@r1viollet
Copy link
Collaborator

I think we left things on whether to namespace or not some of these variables. The thought was that:

  • If we namespace the variables, we can always revert this (or introduce new logics based on feedback).
  • If we do not namespace and there are conflicts on hosts (between profiled service and host), it will be hard to re-introduce the namespaced variables.

@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch 3 times, most recently from 0212176 to 7635875 Compare October 24, 2024 10:35
@nsavoire nsavoire requested review from Gandem and r1viollet October 25, 2024 13:50
@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch 2 times, most recently from e4e3270 to 56ac5bc Compare October 25, 2024 14:11
cli_flags.go Outdated Show resolved Hide resolved
cli_flags.go Outdated Show resolved Hide resolved
@nsavoire nsavoire force-pushed the nsavoire/rename_env_variables branch from acbc8f8 to c25d41e Compare November 4, 2024 11:02
@nsavoire nsavoire requested review from Gandem and r1viollet November 4, 2024 14:31
Copy link
Member

@Gandem Gandem left a comment

Choose a reason for hiding this comment

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

Thank you!

@nsavoire nsavoire merged commit 1d2dd25 into main Nov 5, 2024
10 checks passed
@nsavoire nsavoire deleted the nsavoire/rename_env_variables branch November 5, 2024 14:56
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