-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix automatic APM instrumentation for JS tracer. Add diagnostic logging to APM configurators. Update APM instrumentation docs #388
Conversation
…ng to APM configurators. Update APM instrumentation docs
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.
Dropped some comments. The logic to install the JS tracer escapes my knowledge, so maybe it's worth it to double-check with the libraries team.
|
||
@Override | ||
public Map<String, String> configure(DatadogTracerJobProperty<?> tracerConfig, Node node, FilePath workspacePath, Map<String, String> envs, TaskListener listener) throws Exception { | ||
String nodeVersion = workspacePath.act(new ShellCommandCallable(Collections.emptyMap(), GET_NPM_VERSION_TIMEOUT_MILLIS, "npm", "-v")); |
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.
If npm
is not installed, would this return an exception?
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.
Yep, it'll throw an exception that will be caught and logged in the calling class
listener.getLogger().println("[datadog] Configuring DD JS tracer: got npm version " + nodeVersion + " from " + workspacePath + " on " + node); | ||
|
||
FilePath datadogPath = workspacePath.child(".datadog"); | ||
datadogPath.mkdirs(); |
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.
Do we know if the plugin has permissions to create folders?
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.
It has, and we only do things inside the job's workspace
|
||
// we install dd-trace as a "global" package | ||
// (otherwise, doing SCM checkout might rollback the changes to package.json, and any subsequent `npm install` calls will result in removing the package) | ||
String installTracerOutput = workspacePath.act(new ShellCommandCallable(environment, INSTALL_TRACER_TIMEOUT_MILLIS, "npm", "install", "-g", "dd-trace")); |
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.
Could we have permissions issues?
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 "global" package naming is a bit misleading here - what this means is we are not installing it inside the repo (because subsequent SCM checkout would overwrite this) and instead we install it inside the .datadog
folder which we create inside the job's workspace - where we are allowed to write.
We could have a permissions issue if the OS user that runs the Jenkins agent is not allowed to execute npm
, but I don't think there's anything we can do in this case other than log an error.
Requirements for Contributing to this repository
What does this PR do?
Description of the Change
Alternate Designs
Possible Drawbacks
Verification Process
Additional Notes
Release Notes
Review checklist (to be filled by reviewers)
changelog/
label attached. If applicable it should have thebackward-incompatible
label attached.do-not-merge/
label attached.kind/
andseverity/
labels attached at least.