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.

Sign up for GitHub

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

Avoid calculating _dd.hostname for Jenkins workers #184

Merged
merged 3 commits into from
Feb 22, 2021
Merged

Avoid calculating _dd.hostname for Jenkins workers #184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
822226e
Fix _dd.hostname for traces
drodriguezhdez Feb 19, 2021
3eda94e
Add more comments
drodriguezhdez Feb 22, 2021
578dd14
Remove unnecesary imports
drodriguezhdez Feb 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/main/java/org/datadog/jenkins/plugins/datadog/traces/DatadogTraceBuildLogic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
*/
public class DatadogTraceBuildLogic {

private static final String HOSTNAME_NONE = "none";
private static final Logger logger = Logger.getLogger(DatadogTraceBuildLogic.class.getName());

private final Tracer tracer;
Expand Down Expand Up @@ -123,9 +124,11 @@ public void finishBuildTrace(final BuildData buildData, final Run<?,?> run) {

final String nodeName = buildData.getNodeName("").isEmpty() ? pipelineData.getNodeName("") : buildData.getNodeName("");
buildSpan.setTag(CITags.NODE_NAME, nodeName);

final String nodeHostname = buildData.getHostname("").isEmpty() ? pipelineData.getHostname("") : buildData.getHostname("");
buildSpan.setTag(CITags._DD_HOSTNAME, nodeHostname);
// If the NodeName == master, we don't set _dd.hostname. It will be overridden by the Datadog Agent. (Traces are only available using Datadog Agent)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's not true when the plugin is configured in HTTP mode. In that case, no hostname will be reported.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, Jenkins traces collection is only available if the plugin is configured in a Datadog Agent mode: (link), so we can assume that's correct, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, completely forgot about that. Was there any plan to allow sending traces via HTTP? If yes we should add some additional logic here to fail if the plugin runs in http mode. That will help in the future

// If the NodeName != master, we set _dd.hostname to 'none' explicitly, cause we cannot calculate the worker hostname.
if(!"master".equalsIgnoreCase(nodeName)) {
buildSpan.setTag(CITags._DD_HOSTNAME, HOSTNAME_NONE);
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the impact of setting none here? This has a pretty big impact for metrics for instance (as it would create a new host with the "none" name), but this should be for traces only in that situation. Are we ok with setting a dummy value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as we know, APM doesn't create hosts based on the _dd.hostname tag. In this case, the value none is a special value for APM that is excluded from per-host billing. Additionally, if we don't set it and we let the agent put the master hostname, it'd be worse from a user perspective because we will be showing wrong information.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, I didn't know about billing implications

}

// Git Info
final String gitUrl = buildData.getGitUrl("").isEmpty() ? pipelineData.getGitUrl("") : buildData.getGitUrl("");
Expand Down
27 changes: 8 additions & 19 deletions src/main/java/org/datadog/jenkins/plugins/datadog/traces/DatadogTracePipelineLogic.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,25 +350,14 @@ private Map<String, Object> buildTraceTags(final BuildPipelineNode current, fina
final String user = envVars.get("USER") != null ? envVars.get("USER") : buildData.getUserId();
tags.put(CITags.USER_NAME, user);

//Node info
if(current.getNodeName() != null) {
tags.put(CITags.NODE_NAME, current.getNodeName());

String nodeHostname = HOSTNAME_NONE;
if(current.getNodeHostname() != null) {
nodeHostname = current.getNodeHostname();
} else if(current.getNodeName().equals("master")) {
// If the nodeName is master,
// we can set the hostname from the buildData.
nodeHostname = buildData.getHostname("");
}

tags.put(CITags._DD_HOSTNAME, nodeHostname);
} else {
// If there is no node explicitly set for the step,
// we consider that is the node from the build.
tags.put(CITags.NODE_NAME, buildData.getNodeName(""));
tags.put(CITags._DD_HOSTNAME, buildData.getHostname(""));
// Node info
// If there is no node explicitly set for the step, we consider that is the node from the build.
final String nodeName = current.getNodeName() != null ? current.getNodeName() : buildData.getNodeName("");
tags.put(CITags.NODE_NAME, nodeName);
// If the NodeName == "master", we don't set _dd.hostname. It will be overridden by the Datadog Agent. (Traces are only available using Datadog Agent)
// If the NodeName != "master", we set _dd.hostname to 'none' explicitly, cause we cannot calculate the worker hostname.
if(!"master".equalsIgnoreCase(nodeName)){
tags.put(CITags._DD_HOSTNAME, HOSTNAME_NONE);
}

// Arguments
Expand Down
5 changes: 3 additions & 2 deletions src/test/java/org/datadog/jenkins/plugins/datadog/listeners/DatadogBuildListenerIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;

import datadog.trace.common.writer.ListWriter;
Expand All @@ -17,7 +18,6 @@
import org.datadog.jenkins.plugins.datadog.clients.DatadogClientStub;
import org.datadog.jenkins.plugins.datadog.model.BuildPipelineNode;
import org.datadog.jenkins.plugins.datadog.traces.CITags;
import org.datadog.jenkins.plugins.datadog.traces.GitInfoUtils;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
Expand Down Expand Up @@ -74,6 +74,7 @@ public void testTracesQueueTime() throws Exception{
final DDSpan buildSpan = buildTrace.get(0);
long queueTime = (long) buildSpan.getUnsafeMetrics().get(CITags.QUEUE_TIME);
assertTrue(queueTime > 0L);
assertEquals("none", buildSpan.getTag(CITags._DD_HOSTNAME));
}

@Test
Expand Down Expand Up @@ -120,7 +121,7 @@ public void testTraces() throws Exception {
assertEquals("success", buildSpan.getTag(buildPrefix + CITags._RESULT));
assertEquals("success", buildSpan.getTag(CITags.STATUS));
assertNotNull(buildSpan.getTag(CITags.NODE_NAME));
assertNotNull(buildSpan.getTag(CITags._DD_HOSTNAME));
assertNull(buildSpan.getTag(CITags._DD_HOSTNAME));
assertEquals("success", buildSpan.getTag(CITags.JENKINS_RESULT));
assertEquals("jenkins-buildIntegrationSuccess-1", buildSpan.getTag(CITags.JENKINS_TAG));
assertNotNull(buildSpan.getTag(CITags._DD_CI_STAGES));
Expand Down
32 changes: 25 additions & 7 deletions src/test/java/org/datadog/jenkins/plugins/datadog/listeners/DatadogGraphListenerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@
import org.datadog.jenkins.plugins.datadog.clients.DatadogClientStub;
import org.datadog.jenkins.plugins.datadog.model.BuildPipelineNode;
import org.datadog.jenkins.plugins.datadog.traces.CITags;
import org.datadog.jenkins.plugins.datadog.traces.GitInfoUtils;
import org.datadog.jenkins.plugins.datadog.util.git.GitUtils;
import org.jenkinsci.plugins.workflow.actions.LabelAction;
import org.jenkinsci.plugins.workflow.actions.ThreadNameAction;
import org.jenkinsci.plugins.workflow.actions.TimingAction;
Expand Down Expand Up @@ -379,6 +377,10 @@ public void testIntegrationPipelineQueueTimeOnStages() throws Exception {

@Test
public void testIntegrationPipelineQueueTimeOnPipeline() throws Exception {
final EnvironmentVariablesNodeProperty envProps = new EnvironmentVariablesNodeProperty();
EnvVars env = envProps.getEnvVars();
env.put("NODE_NAME", "testPipeline");
jenkinsRule.jenkins.getGlobalNodeProperties().add(envProps);
WorkflowJob job = jenkinsRule.jenkins.createProject(WorkflowJob.class, "pipelineIntegrationQueueTimeOnPipeline");
String definition = IOUtils.toString(
this.getClass().getResourceAsStream("testPipelineQueueOnPipeline.txt"),
Expand Down Expand Up @@ -406,27 +408,43 @@ public void testIntegrationPipelineQueueTimeOnPipeline() throws Exception {
final DDSpan buildSpan = buildTrace.get(0);
long queueTime = (long) buildSpan.getUnsafeMetrics().get(CITags.QUEUE_TIME);
assertTrue(queueTime > 0L);
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final List<DDSpan> pipelineTrace = tracerWriter.get(1);
assertEquals(6, pipelineTrace.size());
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan startPipeline = pipelineTrace.get(0);
assertEquals(queueTime, startPipeline.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan allocateNode = pipelineTrace.get(1);
assertEquals(queueTime, allocateNode.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan allocateNodeStart = pipelineTrace.get(2);
assertEquals(0L, allocateNodeStart.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan stageStart = pipelineTrace.get(3);
assertEquals(0L, stageStart.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan stage = pipelineTrace.get(4);
assertEquals(0L, stage.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));

final DDSpan step = pipelineTrace.get(5);
assertEquals(0L, step.getUnsafeMetrics().get(CITags.QUEUE_TIME));
assertEquals("testPipeline", buildSpan.getTag(CITags.NODE_NAME));
assertEquals("none",buildSpan.getTag(CITags._DD_HOSTNAME));
}

@Test
Expand Down Expand Up @@ -476,7 +494,7 @@ public void testIntegrationNoFailureTag() throws Exception {
assertEquals("success", buildSpan.getTag(CITags.STATUS));
assertNotNull(buildSpan.getTag(buildPrefix + CITags._URL));
assertNotNull(buildSpan.getTag(CITags.NODE_NAME));
assertNotNull(buildSpan.getTag(CITags._DD_HOSTNAME));
assertNull(buildSpan.getTag(CITags._DD_HOSTNAME));
assertEquals("success", buildSpan.getTag(CITags.JENKINS_RESULT));
assertEquals("jenkins-pipelineIntegrationSuccess-1", buildSpan.getTag(CITags.JENKINS_TAG));
assertEquals(false, buildSpan.getTag(CITags._DD_CI_INTERNAL));
Expand All @@ -500,7 +518,7 @@ public void testIntegrationNoFailureTag() throws Exception {
assertEquals("jenkins", pipelineSpan.getTag(CITags.CI_PROVIDER_NAME));
assertNotNull(pipelineSpan.getTag(pipelinePrefix + CITags._URL));
assertNotNull(pipelineSpan.getTag(CITags.NODE_NAME));
assertNotNull(pipelineSpan.getTag(CITags._DD_HOSTNAME));
assertNull(pipelineSpan.getTag(CITags._DD_HOSTNAME));
assertEquals(true, pipelineSpan.getTag(CITags._DD_CI_INTERNAL));
assertNull(pipelineSpan.getTag(CITags._DD_CI_BUILD_LEVEL));
assertNull(pipelineSpan.getTag(CITags._DD_CI_LEVEL));
Expand All @@ -521,7 +539,7 @@ public void testIntegrationNoFailureTag() throws Exception {
assertEquals("test", stepInternalSpan.getTag("jenkins.step.args.name"));
assertNotNull(stepInternalSpan.getTag(stepPrefix + CITags._URL));
assertNotNull(stepInternalSpan.getTag(CITags.NODE_NAME));
assertNotNull(stepInternalSpan.getTag(CITags._DD_HOSTNAME));
assertNull(stepInternalSpan.getTag(CITags._DD_HOSTNAME));
assertEquals(true, stepInternalSpan.getTag(CITags._DD_CI_INTERNAL));
assertEquals("3", stepInternalSpan.getTag(stepPrefix + CITags._NUMBER));
assertNull(stepInternalSpan.getTag(CITags._DD_CI_BUILD_LEVEL));
Expand All @@ -542,7 +560,7 @@ public void testIntegrationNoFailureTag() throws Exception {
assertEquals("jenkins", stageSpan.getTag(CITags.CI_PROVIDER_NAME));
assertNotNull(stageSpan.getTag(stagePrefix + CITags._URL));
assertNotNull(stageSpan.getTag(CITags.NODE_NAME));
assertNotNull(stageSpan.getTag(CITags._DD_HOSTNAME));
assertNull(stageSpan.getTag(CITags._DD_HOSTNAME));
assertEquals(false, stageSpan.getTag(CITags._DD_CI_INTERNAL));
assertEquals("4", stageSpan.getTag(stagePrefix + CITags._NUMBER));
assertEquals(BuildPipelineNode.NodeType.STAGE.getBuildLevel(), stageSpan.getTag(CITags._DD_CI_BUILD_LEVEL));
Expand All @@ -564,7 +582,7 @@ public void testIntegrationNoFailureTag() throws Exception {
assertNotNull(stepAtomSpan.getTag(stepPrefix + CITags._URL));
assertNotNull(stepAtomSpan.getTag(stepPrefix + CITags._URL));
assertNotNull(stepAtomSpan.getTag(CITags.NODE_NAME));
assertNotNull(stepAtomSpan.getTag(CITags._DD_HOSTNAME));
assertNull(stepAtomSpan.getTag(CITags._DD_HOSTNAME));
assertEquals(false, stepAtomSpan.getTag(CITags._DD_CI_INTERNAL));
assertEquals("5", stepAtomSpan.getTag(stepPrefix + CITags._NUMBER));
assertEquals(BuildPipelineNode.NodeType.STEP.getBuildLevel(), stepAtomSpan.getTag(CITags._DD_CI_BUILD_LEVEL));
Expand Down