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

Victorsesan · 2024-10-21T01:16:21Z

…code priorities and description handling

Relate to #6797

…code priorities and description handling Relate to open-telemetry#6797

jack-berg · 2024-10-21T14:40:29Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

-      this.status = StatusData.create(statusCode, description);
-    }
-    return this;
+      synchronized (lock) {


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

jack-berg · 2024-10-21T14:40:56Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

@@ -418,22 +418,37 @@ private void addTimedEvent(EventData timedEvent) {

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


Why remove this?

jack-berg · 2024-10-21T14:44:35Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+
+          // 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.");


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

jack-berg · 2024-10-21T14:47:45Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+          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


#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

}

Relate to open-telemetry#6797

Victorsesan · 2024-10-23T02:04:16Z

@jack-berg I have fix and added the suggestions as required, sorry for the delay and all the hassle i had so many mistake trying to fix and find a way to run ./gradlew spotlessApply which made me rewrite most of the time from scratch. I have spend hours with every trick there is trying to setup gradle and jdk on my console to run spotless but it keeps showing unable to detect jdk 17 and gradle. Please do let me know if there are still any corrections to be made from the new PR and if by chance there is another method i can go about to ensure the consist code formatting on my PR. thanks

jack-berg · 2024-10-23T14:49:23Z

I have spend hours with every trick there is trying to setup gradle and jdk on my console to run spotless but it keeps showing unable to detect jdk 17 and gradle.

Yes the build requires jdk 17, and gradle detects jdk versions in a very specific way as documented here: https://docs.gradle.org/current/userguide/toolchains.html#sec:auto_detection

My personal preference for installing / managing JDKs is to use sdkman!, which makes it super easy to install multiple versions and toggle between them as needed. Gradle will automatically look for jdks that match the project requirements where sdkman installs them.

Victorsesan · 2024-10-24T04:31:12Z

@jack-berg I finally got jdk 17 running with sdkman! I'm very excited! though the setup couldn't have been completed without the help of wsl but still i got it up and ruuning.
Edit: I think it worked this time! please check my recent commits and see the changes i made, if there is any redo or correction that is needed i will be happy to. Thanks for your time

*Gradle build in build.gradle.kts had failed during spotless check because the plugin org.graalvm.buildtools.native was not found -Solution: I have applied a plugin and made it work *spotlessJavaCheck task failed because certain files did not conform to the formatting rules defined in the Spotless configuration -Solution: Initiated an auto fix using ./gradlew :sdk:trace:spotlessApply *Lastly as requested i initiated ./gradlew spotlessApply to ensure a consist code formating and the build was successful with no errors! Relate to open-telemetry#6797

codecov · 2024-10-24T15:56:23Z

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.47%. Comparing base (5ec1e86) to head (7991abf).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
.../main/java/io/opentelemetry/sdk/trace/SdkSpan.java	71.42%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6808      +/-   ##
============================================
+ Coverage     90.38%   90.47%   +0.09%     
- Complexity     6584     6602      +18     
============================================
  Files           731      731              
  Lines         19722    19742      +20     
  Branches       1931     1938       +7     
============================================
+ Hits          17825    17862      +37     
+ Misses         1300     1288      -12     
+ Partials        597      592       -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkwatson · 2024-10-24T23:55:47Z

This PR seems to have a bunch of unrelated changes in it. And, it definitely needs unit tests for the new behavior.

Victorsesan · 2024-10-25T01:53:28Z

This PR seems to have a bunch of unrelated changes in it. And, it definitely needs unit tests for the new behavior.

@jkwatson Is there anything i can do to fix this? I will be very much happy to help. Sorry for all the inconveniences, if there is a way i can run the required unit test so we can have everything back to normal again i will be happy to.

jkwatson · 2024-10-25T02:04:22Z

This PR seems to have a bunch of unrelated changes in it. And, it definitely needs unit tests for the new behavior.

@jkwatson Is there anything i can do to fix this? I will be very much happy to help. Sorry for all the inconveniences, if there is a way i can run the required unit test so we can have everything back to normal again i will be happy to.

You need to write unit tests for the new behavior. Note that the build checks aren't passing because of code coverage.

And, please get the diff into a clean state. Look at the 'Files changed' in the PR and get it into a state where only the changes needed for this PR are there.

jkwatson · 2024-10-25T02:04:50Z

.gitignore

+
+.fake


please revert this change

jkwatson · 2024-10-25T02:05:30Z

buildSrc/build.gradle.kts

+    kotlinGradle {
+        ktlint().editorConfigOverride(mapOf(
+            "indent_size" to "2",
+            "continuation_indent_size" to "2",
+            "max_line_length" to "160",
+            "insert_final_newline" to "true",
+            "ktlint_standard_no-wildcard-imports" to "disabled",
+            "ktlint_standard_max-line-length" to "disabled",
+            "ktlint_standard_trailing-comma-on-call-site" to "disabled",
+            "ktlint_standard_trailing-comma-on-declaration-site" to "disabled",
+            "ktlint_standard_wrapping" to "disabled"
+        ))
+        target("**/*.gradle.kts")
+    }


the comments that were here before were important. Please restore this file to what it was before.

jkwatson · 2024-10-25T02:06:01Z

hs_err_pid13224.log

+
+vm_info: OpenJDK 64-Bit Server VM (17.0.13+11-LTS) for windows-amd64 JRE (17.0.13+11-LTS), built on Oct  9 2024 05:44:29 by "" with MS VC++ 16.10 / 16.11 (VS2019)
+
+END.


please remove this file

Victorsesan · 2024-10-25T23:03:43Z

@jkwatson Seems like we still had a test failure in L437 and 438 in sdk and build should i try to remove them instead?

jkwatson · 2024-10-25T23:19:57Z

buildSrc/build.gradle.kts

-  }
+    toolchain {
+        languageVersion.set(JavaLanguageVersion.of(17))
+    }


do you need changes to this file for some reason? If not, please revert.

jkwatson · 2024-10-25T23:21:01Z

@jkwatson Seems like we still had a test failure in L437 and 438 in sdk and build should i try to remove them instead?

??? The failure is that there is no test coverage. Please add a case to the unit tests for this.

…7 and 438 covered(just for trials)

Victorsesan · 2024-10-26T03:23:28Z

@jkwatson So i have been trying to add a case to the unit test , i also tried adding something like this too in the SdkSpanTest but it didn't even make it pass the build test. Just wanted to show some of the tries i did with a comment before tagging you for some pointers. check it out
`@ Test
void testSetStatusToUnsetWhenCurrentStatusIsError() {
// Create a new SdkSpan instance
SdkSpan testSpan = createTestRootSpan();

// Set the initial status to ERROR
testSpan.setStatus(StatusCode.ERROR, "An error occurred");

// Capture the logger output
Logger logger = Logger.getLogger(SdkSpan.class.getName());
Level originalLevel = logger.getLevel();
logger.setLevel(Level.ALL); // Set to ALL to capture all log messages

// Create a custom log handler to capture log messages
TestLogHandler testLogHandler = new TestLogHandler();
logger.addHandler(testLogHandler);

// Attempt to set the status to UNSET
testSpan.setStatus(StatusCode.UNSET, null);

// Verify that the warning was logged
String lastMessage = testLogHandler.getLastMessage();
assertEquals("Cannot set status to UNSET when current status is ERROR.", lastMessage);

// Clean up
logger.removeHandler(testLogHandler);
logger.setLevel(originalLevel);

}`

jkwatson · 2024-10-26T16:35:16Z

buildSrc/build.gradle.kts

+    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")


please revert all changes to this file.

jkwatson · 2024-10-26T16:38:25Z

@jkwatson So i have been trying to add a case to the unit test , i also tried adding something like this too in the SdkSpanTest but it didn't even make it pass the build test. Just wanted to show some of the tries i did with a comment before tagging you for some pointers. check it out `@ Test void testSetStatusToUnsetWhenCurrentStatusIsError() { // Create a new SdkSpan instance SdkSpan testSpan = createTestRootSpan();
// Set the initial status to ERROR
testSpan.setStatus(StatusCode.ERROR, "An error occurred");

// Capture the logger output
Logger logger = Logger.getLogger(SdkSpan.class.getName());
Level originalLevel = logger.getLevel();
logger.setLevel(Level.ALL); // Set to ALL to capture all log messages

// Create a custom log handler to capture log messages
TestLogHandler testLogHandler = new TestLogHandler();
logger.addHandler(testLogHandler);

// Attempt to set the status to UNSET
testSpan.setStatus(StatusCode.UNSET, null);

// Verify that the warning was logged
String lastMessage = testLogHandler.getLastMessage();
assertEquals("Cannot set status to UNSET when current status is ERROR.", lastMessage);

// Clean up
logger.removeHandler(testLogHandler);
logger.setLevel(originalLevel);
}`

I don't care all that much about asserting the fact that things were logged. Much more important is making sure that the span status hasn't changed. Take a look at some of the other tests in the SdkSpanTest. I bet you'll find one that is testing the various status state changes that you could either add to, or base a new test case on.

…ngChangedAfterEnd test Ref Fix Span#setStatus open-telemetry#6797

jkwatson · 2024-11-05T04:17:39Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

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


I don't think this should be a WARNING. Probably just FINE like we have for the other status changes.

jkwatson · 2024-11-05T04:17:52Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+
+      // Prevent setting a lower priority status.
+      if (currentStatusCode == StatusCode.OK) {
+        return this; // Do not allow lower priority status to override OK


why remove the logging that was here before?

jkwatson · 2024-11-05T04:18:20Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+
+      // Set the status, ignoring description if status is not ERROR.
+      if (statusCode == StatusCode.ERROR) {
+        this.status = StatusData.create(statusCode, description); // Allow description for ERROR


Suggested change

this.status = StatusData.create(statusCode, description); // Allow description for ERROR

this.status = StatusData.create(statusCode, description);

jkwatson · 2024-11-05T04:18:30Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

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


Suggested change

StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses

StatusData.create(statusCode, null);

jkwatson · 2024-11-05T04:18:42Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

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


Suggested change

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

return this;

jkwatson · 2024-11-05T04:19:56Z

sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanTest.java

@@ -119,11 +119,25 @@ void setUp() {
  @Test
  void nothingChangedAfterEnd() {


The change you made doesn't only apply after the span has ended, does it? This test is testing the case of things not changing after span ending.

take a look at the test called setStatusCannotOverrideStatusOK. Write a new test that looks much like that one that tests the case(s) that you've introduced.

jkwatson · 2024-11-05T04:22:35Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

-      } 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


Suggested change

return this; // Prevent modification if the span has ended

return this;

jkwatson · 2024-11-05T04:22:49Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+        return this; // Prevent modification if the span has ended
+      }
+
+      // Check the current status and enforce priority rules


Suggested change

// Check the current status and enforce priority rules

jkwatson · 2024-11-05T04:23:31Z

sdk/trace/src/main/java/io/opentelemetry/sdk/trace/SdkSpan.java

+      // Prevent setting a lower priority status.
+      if (currentStatusCode == StatusCode.OK) {
+        return this; // Do not allow lower priority status to override OK
+      } else if (currentStatusCode == StatusCode.ERROR && statusCode == StatusCode.UNSET) {


this else is redundant, since the if above it returns already.

setStatus method conforms to the specified behavior regarding status …

7f9ef5f

…code priorities and description handling Relate to open-telemetry#6797

Victorsesan requested a review from a team as a code owner October 21, 2024 01:16

jack-berg reviewed Oct 21, 2024

View reviewed changes

Victorsesan and others added 2 commits October 23, 2024 02:35

fix

ef46f8f

Relate to open-telemetry#6797

Merge branch 'open-telemetry:main' into main

9590742

Victorsesan added 2 commits October 24, 2024 16:44

Merge branch 'main' of github.com:Victorsesan/opentelemetry-java

500d261

jkwatson reviewed Oct 25, 2024

View reviewed changes

.gitignore Outdated

Comment on lines 37 to 38

.fake

Copy link

Contributor

jkwatson Oct 25, 2024

Choose a reason for hiding this comment

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

please revert this change

jkwatson reviewed Oct 25, 2024

View reviewed changes

fix comments

aa731c7

jkwatson reviewed Oct 25, 2024

View reviewed changes

Victorsesan added 6 commits October 26, 2024 00:30

fix comment

bed6535

Created a test in the sdkspantest to see if codecov will have line 43…

c21608c

…7 and 438 covered(just for trials)

test 2

ec33889

.

ab67793

.

189c416

.

7991abf

jkwatson reviewed Oct 26, 2024

View reviewed changes

Victorsesan and others added 4 commits November 3, 2024 03:28

ensured span's status does not change after it has ended in the nothi…

448ece8

…ngChangedAfterEnd test Ref Fix Span#setStatus open-telemetry#6797

Merge branch 'open-telemetry:main' into main

5a62252

#

2fdd372

#

d452c3e

jkwatson reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Victorsesan commented Oct 21, 2024

jack-berg Oct 21, 2024

jack-berg Oct 21, 2024

jack-berg Oct 21, 2024

jack-berg Oct 21, 2024

Victorsesan commented Oct 23, 2024

jack-berg commented Oct 23, 2024

Victorsesan commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

jkwatson commented Oct 24, 2024

Victorsesan commented Oct 25, 2024 •

edited

Loading

jkwatson commented Oct 25, 2024

jkwatson Oct 25, 2024

jkwatson Oct 25, 2024

jkwatson Oct 25, 2024

Victorsesan commented Oct 25, 2024

jkwatson Oct 25, 2024

jkwatson commented Oct 25, 2024

Victorsesan commented Oct 26, 2024 •

edited

Loading

jkwatson Oct 26, 2024

jkwatson commented Oct 26, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024

jkwatson Nov 5, 2024


		vm_info: OpenJDK 64-Bit Server VM (17.0.13+11-LTS) for windows-amd64 JRE (17.0.13+11-LTS), built on Oct 9 2024 05:44:29 by "" with MS VC++ 16.10 / 16.11 (VS2019)

		END.

	this.status = StatusData.create(statusCode, description); // Allow description for ERROR
	this.status = StatusData.create(statusCode, description);

	StatusData.create(statusCode, null); // Ignore description for non-ERROR statuses
	StatusData.create(statusCode, null);

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

		@@ -119,11 +119,25 @@ void setUp() {
		@Test
		void nothingChangedAfterEnd() {

	return this; // Prevent modification if the span has ended
	return this;

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

Are you sure you want to change the base?

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

Conversation

Victorsesan commented Oct 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Victorsesan commented Oct 23, 2024

jack-berg commented Oct 23, 2024

Victorsesan commented Oct 24, 2024 • edited Loading

codecov bot commented Oct 24, 2024 • edited Loading

Codecov Report

jkwatson commented Oct 24, 2024

Victorsesan commented Oct 25, 2024 • edited Loading

jkwatson commented Oct 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Victorsesan commented Oct 25, 2024

Choose a reason for hiding this comment

jkwatson commented Oct 25, 2024

Victorsesan commented Oct 26, 2024 • edited Loading

Choose a reason for hiding this comment

jkwatson commented Oct 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Victorsesan commented Oct 24, 2024 •

edited

Loading

codecov bot commented Oct 24, 2024 •

edited

Loading

Victorsesan commented Oct 25, 2024 •

edited

Loading

Victorsesan commented Oct 26, 2024 •

edited

Loading