-
Notifications
You must be signed in to change notification settings - Fork 849
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
base: main
Are you sure you want to change the base?
Changes from 6 commits
7f9ef5f
ef46f8f
9590742
a9fb915
500d261
aa731c7
bed6535
c21608c
ec33889
ab67793
189c416
7991abf
448ece8
5a62252
2fdd372
d452c3e
f04bb8d
5aa430e
db6a054
b1776cf
d9e11e8
8fc9659
cb9283e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,29 +1,28 @@ | ||
plugins { | ||
`kotlin-dsl` | ||
|
||
// When updating, update below in dependencies too | ||
id("com.diffplug.spotless") version "6.25.0" | ||
`kotlin-dsl` | ||
// When updating, update below in dependencies too | ||
id("com.diffplug.spotless") version "6.25.0" | ||
} | ||
|
||
if (!hasLauncherForJavaVersion(17)) { | ||
throw GradleException( | ||
"JDK 17 is required to build and gradle was unable to detect it on the system. " + | ||
"Please install it and see https://docs.gradle.org/current/userguide/toolchains.html#sec:auto_detection " + | ||
"for details on how gradle detects java toolchains." | ||
) | ||
throw GradleException( | ||
"JDK 17 is required to build and gradle was unable to detect it on the system. " + | ||
"Please install it and see https://docs.gradle.org/current/userguide/toolchains.html#sec:auto_detection " + | ||
"for details on how gradle detects java toolchains." | ||
) | ||
} | ||
|
||
fun hasLauncherForJavaVersion(version: Int): Boolean { | ||
return try { | ||
javaToolchains.launcherFor { languageVersion = JavaLanguageVersion.of(version) }.get() | ||
true | ||
} catch (e: Exception) { | ||
false | ||
} | ||
return try { | ||
javaToolchains.launcherFor { languageVersion.set(JavaLanguageVersion.of(version)) }.get() | ||
true | ||
} catch (e: Exception) { | ||
false | ||
} | ||
} | ||
|
||
spotless { | ||
kotlinGradle { | ||
kotlinGradle { | ||
ktlint().editorConfigOverride(mapOf( | ||
"indent_size" to "2", | ||
"continuation_indent_size" to "2", | ||
|
@@ -44,36 +43,33 @@ spotless { | |
} | ||
|
||
repositories { | ||
mavenCentral() | ||
gradlePluginPortal() | ||
mavenLocal() | ||
mavenCentral() | ||
gradlePluginPortal() | ||
mavenLocal() | ||
} | ||
|
||
dependencies { | ||
implementation(enforcedPlatform("com.squareup.wire:wire-bom:5.1.0")) | ||
implementation("com.google.auto.value:auto-value-annotations:1.11.0") | ||
// When updating, update above in plugins too | ||
implementation("com.diffplug.spotless:spotless-plugin-gradle:6.25.0") | ||
// Needed for japicmp but not automatically brought in for some reason. | ||
implementation("com.google.guava:guava:33.3.1-jre") | ||
implementation("com.squareup:javapoet:1.13.0") | ||
implementation("com.squareup.wire:wire-compiler") | ||
implementation("com.squareup.wire:wire-gradle-plugin") | ||
implementation("gradle.plugin.com.google.protobuf:protobuf-gradle-plugin:0.8.18") | ||
implementation("gradle.plugin.io.morethan.jmhreport:gradle-jmh-report:0.9.6") | ||
implementation("me.champeau.gradle:japicmp-gradle-plugin:0.4.3") | ||
implementation("me.champeau.jmh:jmh-gradle-plugin:0.7.2") | ||
implementation("net.ltgt.gradle:gradle-errorprone-plugin:4.0.1") | ||
implementation("net.ltgt.gradle:gradle-nullaway-plugin:2.0.0") | ||
implementation("org.jetbrains.kotlin:kotlin-gradle-plugin:2.0.21") | ||
implementation("org.owasp:dependency-check-gradle:10.0.4") | ||
implementation("ru.vyarus:gradle-animalsniffer-plugin:1.7.1") | ||
implementation(enforcedPlatform("com.squareup.wire:wire-bom:5.1.0")) | ||
implementation("com.google.auto.value:auto-value-annotations:1.11.0") | ||
implementation("com.diffplug.spotless:spotless-plugin-gradle:6.25.0") | ||
implementation("com.google.guava:guava:33.3.1-jre") | ||
implementation("com.squareup:javapoet:1.13.0") | ||
implementation("com.squareup.wire:wire-compiler") | ||
implementation("com.squareup.wire:wire-gradle-plugin") | ||
implementation("gradle.plugin.com.google.protobuf:protobuf-gradle-plugin:0.8.18") | ||
implementation("gradle.plugin.io.morethan.jmhreport:gradle-jmh-report:0.9.6") | ||
implementation("me.champeau.gradle:japicmp-gradle-plugin:0.4.3") | ||
implementation("me.champeau.jmh:jmh-gradle-plugin:0.7.2") | ||
implementation("net.ltgt.gradle:gradle-errorprone-plugin:4.0.1") | ||
implementation("net.ltgt.gradle:gradle-nullaway-plugin:2.0.0") | ||
implementation("org.jetbrains.kotlin:kotlin-gradle-plugin:2.0.21") | ||
implementation("org.owasp:dependency-check-gradle:10.0.4") | ||
implementation("ru.vyarus:gradle-animalsniffer-plugin:1.7.1") | ||
} | ||
|
||
// We can't apply conventions to this build so include important ones such as the Java compilation | ||
// target. | ||
java { | ||
toolchain { | ||
languageVersion.set(JavaLanguageVersion.of(17)) | ||
} | ||
toolchain { | ||
languageVersion.set(JavaLanguageVersion.of(17)) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you need changes to this file for some reason? If not, please revert. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -419,19 +419,36 @@ | |
@Override | ||
public ReadWriteSpan setStatus(StatusCode statusCode, @Nullable String description) { | ||
if (statusCode == null) { | ||
return this; | ||
return this; // No action if statusCode is null | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
return this; // Prevent modification if the span has ended | ||
Victorsesan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
// Check the current status and enforce priority rules | ||
Victorsesan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
StatusCode currentStatusCode = this.status.getStatusCode(); | ||
|
||
// Prevent setting a lower priority status | ||
if (currentStatusCode == StatusCode.OK) { | ||
return this; // Do not allow lower priority status to override OK | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why remove the logging that was here before? |
||
} else if (currentStatusCode == StatusCode.ERROR && statusCode == StatusCode.UNSET) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this |
||
logger.log(Level.WARNING, "Cannot set status to UNSET when current status is ERROR."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be a WARNING. Probably just FINE like we have for the other status changes. |
||
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 | ||
Victorsesan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} else { | ||
if (currentStatusCode != statusCode) { | ||
this.status = | ||
StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses | ||
Victorsesan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
this.status = StatusData.create(statusCode, description); | ||
} | ||
return this; | ||
return this; // Return the current span for method chaining | ||
Victorsesan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert all changes to this file.