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

setStatus method conforms to the specified behavior regarding status … #6808

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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 .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,5 @@ bin

# Vim
.swp

.fake
43 changes: 29 additions & 14 deletions sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java
Original file line number Diff line number Diff line change
Expand Up @@ -418,22 +418,37 @@ private void addTimedEvent(EventData timedEvent) {

@Override
public ReadWriteSpan setStatus(StatusCode statusCode, @Nullable String description) {
if (statusCode == null) {
return this;
}
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this?

synchronized (lock) {
if (!isModifiableByCurrentThread()) {
logger.log(Level.FINE, "Calling setStatus() on an ended Span.");
return this;
} else if (this.status.getStatusCode() == StatusCode.OK) {
logger.log(Level.FINE, "Calling setStatus() on a Span that is already set to OK.");
return this;
if (statusCode == null) {
return this; // No action if statusCode is null
}
this.status = StatusData.create(statusCode, description);
}
return this;
synchronized (lock) {
Copy link
Member

Choose a reason for hiding this comment

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

We use spotless to ensure consist code formatting. Please run ./gradlew spotlessApply.

if (!isModifiableByCurrentThread()) {
logger.log(Level.FINE, "Calling setStatus() on an ended Span.");
return this; // Prevent modification if the span has ended
}

// Check the current status and enforce priority rules
StatusCode currentStatusCode = this.status.getStatusCode();

// Prevent setting a lower priority status
if (currentStatusCode == StatusCode.OK) {
logger.log(Level.FINE, "Calling setStatus() on a Span that is already set to OK.");
Copy link
Member

Choose a reason for hiding this comment

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

Only log a warning the target status code is != StatusCode.OK.

return this; // Do not allow lower priority status to override OK
} else if (currentStatusCode == StatusCode.ERROR && statusCode == StatusCode.UNSET) {
logger.log(Level.FINE, "Cannot set status to UNSET when current status is ERROR.");
return this; // Do not allow UNSET to override ERROR
}

// Set the status, ignoring description if status is not ERROR
if (statusCode == StatusCode.ERROR) {
this.status = StatusData.create(statusCode, description); // Allow description for ERROR
} else {
this.status = StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses
Copy link
Member

Choose a reason for hiding this comment

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

#nit: Let's skip the extra allocation if the status code doesn't stand to change.

Suggested change
this.status = StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses
if (currentStatusCode != statusCode) {
this.status = StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses
}

}
}
return this; // Return the current span for method chaining
}

@Override
public ReadWriteSpan recordException(Throwable exception) {
recordException(exception, Attributes.empty());
Expand Down
Loading