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 16 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 @@ -30,6 +30,8 @@ endif::[]
* Add support for Vert.x web client- {pull}1824[#1824]
* Avoid recycling of spans and transactions that are using through the public API, so to avoid
reference-counting-related errors - {pull}1859[#1859]
* 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 @@ -160,4 +160,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 @@ -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;
}
}
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 @@ -424,4 +424,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 @@ -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
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 @@ -41,19 +41,46 @@ public class Destination implements Recyclable {
*/
private final StringBuilder address = new StringBuilder();

/**
* A custom address. If set, takes precedence over the automatically discovered one.
*/
private final StringBuilder userAddress = new StringBuilder();

private boolean ignoreAddress;

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

/**
* A custom port. If set, takes precedence over the automatically discovered one.
*/
private int userPort;

private boolean ignorePort;

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

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

public StringBuilder getAddress() {
if (userAddress.length() > 0 || ignoreAddress) {
return userAddress;
}
return address;
}

Expand All @@ -62,7 +89,20 @@ public Destination withPort(int port) {
return this;
}

public Destination withUserPort(int port) {
if (port > 0) {
userPort = port;
} else {
ignorePort = true;
userPort = 0;
}
return this;
}

public int getPort() {
if (userPort > 0 || ignorePort) {
return userPort;
}
return port;
}

Expand All @@ -78,8 +118,8 @@ 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);
appendAddress(addressPort, 0, separator, this.address);
}
}
}
Expand All @@ -102,13 +142,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 appendAddress(CharSequence address, int start, int end, StringBuilder addressField) {
if (address.length() > 0 && start < end) {
int startIndex = start;
int endIndex = end - 1;
Expand All @@ -120,14 +154,13 @@ private Destination withAddress(CharSequence address, int start, int end) {
}

if (startIndex < endIndex) {
if (this.address.length() > 0) {
if (addressField.length() > 0) {
// buffer reset required if it has already been used
this.address.delete(0, this.address.length());
addressField.delete(0, addressField.length());
}
this.address.append(address, startIndex, endIndex + 1);
addressField.append(address, startIndex, endIndex + 1);
}
}
return this;
}

/**
Expand All @@ -140,13 +173,17 @@ public Service getService() {
}

public boolean hasContent() {
return address.length() > 0 || port > 0 || service.hasContent();
return userAddress.length() > 0 || address.length() > 0 || userPort > 0 || port > 0 || service.hasContent();
}

@Override
public void resetState() {
address.setLength(0);
userAddress.setLength(0);
ignoreAddress = false;
port = 0;
userPort = 0;
ignorePort = false;
service.resetState();
}

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

/**
* Same as {@link #resource} but used for custom/manual setting. If set, it should always take precedence
* over {@link #resource}.
*/
private final StringBuilder userResource = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

Must be reset in resetState()


private boolean ignoreResource;

/**
* 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()) {
ignoreResource = true;
userResource.setLength(0);
} else {
setNewValue(this.userResource, resource);
}
return this;
}

public Service withResource(String resource) {
this.resource.append(resource);
setNewValue(this.resource, resource);
return this;
}

private void setNewValue(StringBuilder resource, String newValue) {
if (resource.length() > 0) {
resource.setLength(0);
}
resource.append(newValue);
}

public StringBuilder getResource() {
if (userResource.length() > 0 || ignoreResource) {
return userResource;
}
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 || userResource.length() > 0;
}

@Override
public void resetState() {
resource.setLength(0);
userResource.setLength(0);
ignoreResource = false;
name.setLength(0);
type = null;
}
Expand Down
Loading