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 all 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
2 changes: 2 additions & 0 deletions CHANGELOG.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ setting `disable_instrumentations=jdbc` would disable jdbc and also enable exper
We cannot upgrade to version above 2.12.1 because this is the last version of log4j that is compatible with Java 7.
Instead, we exclude the SMTP appender (which is the vulnerable one) from our artifacts. Note that older versions of
our agent are not vulnerable as well, as the SMTP appender was never used, this is only to further reduce our users' concerns.
* Adding public APIs for setting `destination.service.resource`, `destination.address` and `destination.port` fields
for exit spans - {pull}1788[#1788]

[float]
===== Bug fixes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,4 +154,12 @@ public void injectTraceHeaders(HeaderInjector headerInjector) {
private void doInjectTraceHeaders(MethodHandle addHeader, HeaderInjector headerInjector) {
// co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation.InjectTraceHeadersInstrumentation
}

protected void doSetDestinationAddress(@Nullable String address, int port) {
// co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation.SetDestinationAddressInstrumentation
}

protected void doSetDestinationService(@Nullable String resource) {
// co.elastic.apm.agent.pluginapi.AbstractSpanInstrumentation.SetDestinationServiceInstrumentation
}
}
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 @@ -159,4 +159,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 @@ -199,4 +199,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;
}
}
30 changes: 30 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 @@ -418,4 +418,34 @@ public interface Span {
*/
void injectTraceHeaders(HeaderInjector headerInjector);

/**
* Provides a way to manually set the destination address and port for this span. If used, values will override
* the automatically discovered ones, if there are such.
* Any value set through this method will take precedence over the automatically discovered one.
*
* NOTE: this is only relevant for spans that represent outgoing communication. Trying to invoke this method
* on a {@link Transaction} object will result with an {@link UnsupportedOperationException}.
*
* @param address a string representation of the destination host name or IP. {@code null} and empty values will
* will cause the exclusion of the address field from the span context.
* @param port the destination port. Non-positive values will cause the exclusion of the port field from the span context.
* @return this span
*/
@Nonnull
Span setDestinationAddress(@Nullable String address, int port);

/**
* Provides a way to manually set the {@code destination.service.resource} field, which is used for the construction
* of service maps and the identification of downstream services.
* Any value set through this method will take precedence over the automatically inferred one.
*
* NOTE: this is only relevant for spans that represent outgoing communication. Trying to invoke this method
* on a {@link Transaction} object will result with an {@link UnsupportedOperationException}.
*
* @param resource the string representation of the downstream service. Will be used to override automatically
* inferred value, even if {@code null}.
* @return this span
*/
@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 @@ -19,6 +19,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 @@ -110,4 +111,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 @@ -19,6 +19,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 @@ -158,4 +159,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
throw new UnsupportedOperationException();
}

/**
* @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
throw new UnsupportedOperationException();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,29 @@ public class Destination implements Recyclable {
*/
private final StringBuilder address = new StringBuilder();

private boolean addressSetByUser;

/**
* The destination's port used within this context.
*/
private int port;

private boolean portSetByUser;

public Destination withAddress(@Nullable CharSequence address) {
if (address != null) {
return withAddress(address, 0, address.length());
if (address != null && !addressSetByUser) {
withAddress(address, 0, address.length());
}
return this;
}

public Destination withUserAddress(@Nullable CharSequence address) {
if (address == null || address.length() == 0) {
this.address.setLength(0);
} else {
withAddress(address, 0, address.length());
}
addressSetByUser = true;
return this;
}

Expand All @@ -52,7 +66,15 @@ public StringBuilder getAddress() {
}

public Destination withPort(int port) {
this.port = port;
if (!portSetByUser) {
this.port = port;
}
return this;
}

public Destination withUserPort(int port) {
withPort(port);
portSetByUser = true;
return this;
}

Expand All @@ -72,8 +94,10 @@ public Destination withAddressPort(@Nullable String addressPort) {
int port = parsePort(addressPort, separator + 1, addressPort.length());

if (port > 0) {
return withPort(port)
.withAddress(addressPort, 0, separator);
withPort(port);
if (!addressSetByUser) {
withAddress(addressPort, 0, separator);
}
}
}
}
Expand All @@ -96,13 +120,7 @@ private static int parsePort(CharSequence input, int start, int end) {
return port;
}

/**
* @param address address buffer
* @param start address start (inclusive)
* @param end address end (exclusive)
* @return destination with updated address
*/
private Destination withAddress(CharSequence address, int start, int end) {
private void withAddress(CharSequence address, int start, int end) {
if (address.length() > 0 && start < end) {
int startIndex = start;
int endIndex = end - 1;
Expand All @@ -121,7 +139,6 @@ private Destination withAddress(CharSequence address, int start, int end) {
this.address.append(address, startIndex, endIndex + 1);
}
}
return this;
}

/**
Expand All @@ -140,7 +157,9 @@ public boolean hasContent() {
@Override
public void resetState() {
address.setLength(0);
addressSetByUser = false;
port = 0;
portSetByUser = false;
service.resetState();
}

Expand Down Expand Up @@ -179,57 +198,83 @@ public static class Service implements Recyclable {
*/
private final StringBuilder resource = new StringBuilder();

private boolean resourceSetByUser;

/**
* Used for detecting “sameness” of services and then the display name of a service in the Service Map.
* In other words, the {@link Service#resource} is used to query data for ALL destinations. However,
* some `resources` may be nodes of the same cluster, in which case we also want to be aware.
* 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 withUserResource(@Nullable String resource) {
if (resource == null || resource.isEmpty()) {
this.resource.setLength(0);
} else {
setResourceValue(resource);
}
resourceSetByUser = true;
return this;
}

public Service withResource(String resource) {
this.resource.append(resource);
if (!resourceSetByUser) {
setResourceValue(resource);
}
return this;
}

private void setResourceValue(String newValue) {
resource.setLength(0);
resource.append(newValue);
}

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
public void resetState() {
resource.setLength(0);
resourceSetByUser = false;
name.setLength(0);
type = null;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -851,25 +851,36 @@ private void serializeDestination(Destination destination) {
if (destination.hasContent()) {
writeFieldName("destination");
jw.writeByte(OBJECT_START);
if (destination.getAddress().length() > 0) {
writeField("address", destination.getAddress());
boolean hasAddress = destination.getAddress().length() > 0;
boolean hasPort = destination.getPort() > 0;
boolean hasServiceContent = destination.getService().hasContent();
if (hasAddress) {
if (hasPort || hasServiceContent) {
writeField("address", destination.getAddress());
} else {
writeLastField("address", destination.getAddress());
}
}
if (destination.getPort() > 0) {
writeField("port", destination.getPort());
if (hasPort) {
if (hasServiceContent) {
writeField("port", destination.getPort());
} else {
writeLastField("port", destination.getPort());
}
}
serializeService(destination.getService());
serializeService(hasServiceContent, destination.getService());
jw.writeByte(OBJECT_END);
jw.writeByte(COMMA);
}
}

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

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

void writeField(final String fieldName, final StringBuilder value) {
if (value.length() > 0) {
writeFieldName(fieldName);
Expand Down
Loading