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

Issue 1699 extend destination field #1788

Merged
merged 23 commits into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
28086e9
feature added set destination address and service methods for public api
videnkz Apr 25, 2021
6a81c1a
Merge remote-tracking branch 'upstream/master' into issue-1699-extend…
videnkz Apr 25, 2021
a4a563e
added instrumentation for setDestinationAddress, setDestinationService
videnkz Apr 26, 2021
9054031
added more tests
videnkz Apr 26, 2021
dd9bb1f
Merge remote-tracking branch 'upstream/master' into issue-1699-extend…
videnkz Apr 27, 2021
0b81e3a
Merge remote-tracking branch 'upstream/master' into issue-1699-extend…
videnkz Apr 28, 2021
41be722
mark Destination.Service name and type as deprecated. Deleted this fi…
videnkz Apr 28, 2021
ac22962
fixes according to comments
videnkz Apr 29, 2021
c8afc98
Merge branch 'master' into issue-1699-extend-destination-field
eyalkoren Jun 7, 2021
f521832
fixes according to comments
videnkz Jun 9, 2021
2bcfcaf
added userResource to Destination.Service
videnkz Jun 9, 2021
8ee6a2b
Merge branch 'master' into issue-1699-extend-destination-field
videnkz Jun 9, 2021
abaa5d7
Adjusting to updated spec
eyalkoren Jun 20, 2021
3e6410b
Merge remote-tracking branch 'upstream/master' into issue-1699-extend…
eyalkoren Jun 20, 2021
69178ba
Adjusting API test
eyalkoren Jun 20, 2021
f318d39
Adding to docs and CHANGELOG
eyalkoren Jun 20, 2021
69360a3
Applying review suggestion
eyalkoren Jun 21, 2021
45e11eb
Merge remote-tracking branch 'upstream/master' into issue-1699-extend…
eyalkoren Jun 21, 2021
806193c
add few extra destination tests
SylvainJuge Jun 22, 2021
5af4069
minor renaming
SylvainJuge Jun 22, 2021
fbce153
Merge branch 'master' into issue-1699-extend-destination-field
SylvainJuge Jun 22, 2021
b5b7a4a
Merge branch 'master' into issue-1699-extend-destination-field
SylvainJuge Jun 22, 2021
14805b3
Merge branch 'master' into issue-1699-extend-destination-field
eyalkoren Jun 23, 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
Original file line number Diff line number Diff line change
Expand Up @@ -155,4 +155,12 @@ public void injectTraceHeaders(HeaderInjector headerInjector) {
private void doInjectTraceHeaders(MethodHandle addHeader, HeaderInjector headerInjector) {
// co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation.InjectTraceHeadersInstrumentation
}

public void doSetDestinationAddress(@Nullable String address, int port) {

}

public void doSetDestinationService(@Nullable String resource) {

}
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
}
12 changes: 12 additions & 0 deletions apm-agent-api/src/main/java/co/elastic/apm/api/NoopSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -165,4 +165,16 @@ public Span setOutcome(Outcome outcome) {
public void injectTraceHeaders(HeaderInjector headerInjector) {
// noop
}

@Nonnull
@Override
public Span setDestinationAddress(@Nullable String address, int port) {
return this;
}

@Nonnull
@Override
public Span setDestinationService(@Nullable String resource) {
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -205,4 +205,16 @@ public Transaction setOutcome(Outcome outcome) {
public void injectTraceHeaders(HeaderInjector headerInjector) {
// noop
}

@Nonnull
@Override
public Span setDestinationAddress(@Nullable String address, int port) {
return this;
}

@Nonnull
@Override
public Span setDestinationService(@Nullable String resource) {
return this;
}
}
5 changes: 5 additions & 0 deletions apm-agent-api/src/main/java/co/elastic/apm/api/Span.java
Original file line number Diff line number Diff line change
Expand Up @@ -424,4 +424,9 @@ public interface Span {
*/
void injectTraceHeaders(HeaderInjector headerInjector);

@Nonnull
Span setDestinationAddress(@Nullable String address, int port);

@Nonnull
Span setDestinationService(@Nullable String resource);
}
15 changes: 15 additions & 0 deletions apm-agent-api/src/main/java/co/elastic/apm/api/SpanImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
package co.elastic.apm.api;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/**
* If the agent is active, it injects the implementation into this class.
Expand Down Expand Up @@ -116,4 +117,18 @@ public Span setOutcome(Outcome outcome) {
doSetOutcome(outcome);
return this;
}

@Nonnull
@Override
public Span setDestinationAddress(@Nullable String address, int port) {
doSetDestinationAddress(address, port);
return this;
}

@Nonnull
@Override
public Span setDestinationService(@Nullable String resource) {
doSetDestinationService(resource);
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
package co.elastic.apm.api;

import javax.annotation.Nonnull;
import javax.annotation.Nullable;

/**
* If the agent is active, it injects the implementation from
Expand Down Expand Up @@ -164,4 +165,24 @@ public Transaction setOutcome(Outcome outcome) {
doSetOutcome(outcome);
return this;
}

/**
* @deprecated - used only for {@link co.elastic.apm.api.Span}
*/
@Nonnull
@Override
@Deprecated
public Transaction setDestinationAddress(@Nullable String address, int port) {
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
return this;
}

/**
* @deprecated - used only for {@link co.elastic.apm.api.Span}
*/
@Nonnull
@Override
@Deprecated
public Transaction setDestinationService(@Nullable String resource) {
SylvainJuge marked this conversation as resolved.
Show resolved Hide resolved
return this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -192,16 +192,23 @@ public static class Service implements Recyclable {
* Eventually, we may decide to actively fetch a cluster name or similar and we could use that to detect "sameness".
* For now, for HTTP we use scheme, host, and non-default port. For anything else, we use {@code span.subtype}
* (for example- postgresql, elasticsearch).
*
* @deprecated will be removed
*/
private final StringBuilder name = new StringBuilder();

/**
* For displaying icons or similar. Currently, this should be equal to the {@code span.type}.
*
* @deprecated will be removed
*/
@Nullable
private String type;

public Service withResource(String resource) {
if (this.resource.length() > 0) {
this.resource.setLength(0);
}
this.resource.append(resource);
return this;
}
Expand All @@ -210,27 +217,29 @@ public StringBuilder getResource() {
return resource;
}

@Deprecated
public Service withName(String name) {
this.name.append(name);
return this;
}

@Deprecated
public StringBuilder getName() {
return name;
}

public Service withType(String type) {
this.type = type;
@Deprecated
public Service withType(@Nullable String type) {
return this;
}

@Nullable
@Deprecated
public String getType() {
return type;
}

public boolean hasContent() {
return resource.length() > 0 || name.length() > 0 || type != null;
return resource.length() > 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

userResource should be taken into account as well

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -857,25 +857,31 @@ private void serializeDestination(Destination destination) {
if (destination.hasContent()) {
writeFieldName("destination");
jw.writeByte(OBJECT_START);
boolean isNeedAddComma = false;
if (destination.getAddress().length() > 0) {
writeField("address", destination.getAddress());
isNeedAddComma = true;
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
}
if (destination.getPort() > 0) {
writeField("port", destination.getPort());
writeLastField("port", destination.getPort());
isNeedAddComma = true;
}
serializeService(destination.getService());
serializeService(isNeedAddComma, destination.getService());
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
jw.writeByte(OBJECT_END);
jw.writeByte(COMMA);
}
}

private void serializeService(Destination.Service service) {
private void serializeService(boolean isNeedAddComma, Destination.Service service) {
if (service.hasContent()) {
if (isNeedAddComma) {
jw.writeByte(COMMA);
}
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
writeFieldName("service");
jw.writeByte(OBJECT_START);
writeField("name", service.getName());
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
writeEmptyField("name");
writeField("resource", service.getResource());
writeLastField("type", service.getType());
writeLastEmptyField("type");
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
jw.writeByte(OBJECT_END);
}
}
Expand Down Expand Up @@ -1238,6 +1244,17 @@ private void serializeUser(final User user) {
jw.writeByte(OBJECT_END);
}

void writeEmptyField(final String fieldName) {
writeFieldName(fieldName);
writeStringValue("");
jw.writeByte(COMMA);
}

void writeLastEmptyField(final String fieldName) {
writeFieldName(fieldName);
writeStringValue("");
}

eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
void writeField(final String fieldName, final StringBuilder value) {
if (value.length() > 0) {
writeFieldName(fieldName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,7 @@ void testSpanDestinationContextSerialization() {
Span span = new Span(MockTracer.create());
span.getContext().getDestination().withAddress("whatever.com").withPort(80)
.getService()
.withName("http://whatever.com")
.withResource("whatever.com:80")
.withType("external");
.withResource("whatever.com:80");

JsonNode spanJson = readJsonString(serializer.toJsonString(span));
JsonNode context = spanJson.get("context");
Expand All @@ -367,9 +365,9 @@ void testSpanDestinationContextSerialization() {
assertThat(80).isEqualTo(destination.get("port").intValue());
JsonNode service = destination.get("service");
assertThat(service).isNotNull();
assertThat("http://whatever.com").isEqualTo(service.get("name").textValue());
assertThat("whatever.com:80").isEqualTo(service.get("resource").textValue());
assertThat("external").isEqualTo(service.get("type").textValue());
assertThat(service.get("name").textValue()).isEqualTo("");
assertThat(service.get("type").textValue()).isEqualTo("");
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
*/
package co.elastic.apm.agent.pluginapi;

import co.elastic.apm.agent.impl.context.AbstractContext;
import co.elastic.apm.agent.impl.context.Destination;
import co.elastic.apm.agent.impl.context.SpanContext;
import co.elastic.apm.agent.impl.transaction.AbstractSpan;
import co.elastic.apm.agent.impl.transaction.Outcome;
import co.elastic.apm.agent.impl.transaction.Span;
Expand Down Expand Up @@ -369,4 +372,47 @@ public static void injectTraceHeaders(@Advice.FieldValue(value = "span", typing
}
}
}

public static class SetDestinationAddressInstrumentation extends AbstractSpanInstrumentation {

public SetDestinationAddressInstrumentation() {
super(named("doSetDestinationAddress"));
}

@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
public static void setDestinationAddress(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) Object context,
@Advice.Argument(0) @Nullable String address,
@Advice.Argument(1) int port) {
if (context instanceof Span) {
SpanContext spanContext = ((Span) context).getContext();
if (address != null && port > 0 && !address.isBlank()) {
spanContext.getDestination().withAddress(address).withPort(port);
}
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

public static class SetDestinationServiceInstrumentation extends AbstractSpanInstrumentation {

public SetDestinationServiceInstrumentation() {
super(named("doSetDestinationService"));
}

@Advice.OnMethodExit(suppress = Throwable.class, inline = false)
public static void setDestinationService(@Advice.FieldValue(value = "span", typing = Assigner.Typing.DYNAMIC) Object context,
@Advice.Argument(0) @Nullable String resource) {
if (context instanceof Span) {
SpanContext spanContext = ((Span) context).getContext();
boolean isEmptyResource = resource == null || resource.isEmpty();
if (isEmptyResource) {
resource = "";
}
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
Destination.Service service = spanContext.getDestination().getService();
// in case when is already auto detected, and with non-empty values - we override
if (!service.hasContent() || service.hasContent() && !isEmptyResource) {
service.withResource(resource);
}
eyalkoren marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$AddBooleanLabelInstru
co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$ActivateInstrumentation
co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$IsSampledInstrumentation
co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$InjectTraceHeadersInstrumentation
co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$SetDestinationAddressInstrumentation
co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation$SetDestinationServiceInstrumentation
co.elastic.apm.agent.pluginapi.CaptureExceptionInstrumentation
co.elastic.apm.agent.pluginapi.ApiScopeInstrumentation
co.elastic.apm.agent.pluginapi.CaptureTransactionInstrumentation
Expand Down
Loading