-
Notifications
You must be signed in to change notification settings - Fork 536
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 💯
Codecov Report
@@ Coverage Diff @@
## develop #707 +/- ##
===========================================
- Coverage 52.70% 52.68% -0.02%
===========================================
Files 130 130
Lines 17146 17146
===========================================
- Hits 9037 9034 -3
- Misses 7461 7463 +2
- Partials 648 649 +1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me, WHy are we having issues with CLAssistant first time seeing this
Can you add a comment: I have read the CLA Document and I hereby sign the CLA |
c4a1046
to
4a7d5f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach looks good in general, but I don't feel confortable with adding features using only env vars, I mean, ideally for me, this can be configurable in CLI params, config file and env vars.
I agree as well. Let's include the CLI and config equivalent to this PR. @ZeljkoBenovic Ideally, we can offload the implementation to https://github.com/spf13/viper @zivkovicmilos 👀 |
@vcastellm The primary idea was not to clutter the UX with additional flags as DataDog is not a feature that is so commonly used as opposed to Prometheus for example. If you guys think that we need flags/config file parameters for this, it is very easy to add them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
# Conflicts: # go.mod # go.sum
This PR adds Datadog profiler into the codebase, so that it can be easily enabled on demand.
Profiler enables users to see profiles and tracing stats on their Datadog portal, but it requires some configuration of Datadog agent on the node.
APM ( tracing ) should be enabled in Datadog agent configuration yaml file:
To enable the profiler, a user must simply set
DD_PROFILING_ENABLED="True"
environment variable.To set the IP and port that the DataDog agent, use
DD_AGENT_HOST
andDD_TRACE_AGENT_PORT
environment variables.By default, it will try communicate with the agent on
localhost:8126
.These env vars are now consistent with the DD doc https://docs.datadoghq.com/tracing/trace_collection/dd_libraries/go/?tab=containers#configure-the-datadog-agent-for-apm
Additional parameters can be set by using env vars stated in the official Datadog doc https://docs.datadoghq.com/profiler/enabling/go/
Changes include
Checklist
Testing
Manual tests
Manual tests would require that the tester has access to Datadog portal.
Just set the
DD_ENABLE
env var, configure Datadog agent on the same node, and check Datadog portal if you can see your profile.Documentation update
Public doc will soon with the explanation on how to set this up will follow
Additional comments
Fixes EDGE-764