Refactor Event API to reflect spec changes by jack-berg · Pull Request #6318 · open-telemetry/opentelemetry-java

jack-berg · 2024-03-21T22:05:25Z

Reflects changes in open-telemetry/opentelemetry-specification#3772 and open-telemetry/opentelemetry-specification#3749.

Previously attempted in #6001.

Summary of changes:

Restructure API to put payload into log record AnyValue body. There are now a series of put(String key, ? value) types added which add information to the body. The spec currently seems to be thinking that fields in an event body are not attributes. Because of this, there are not overloads for put which accept the attributes.
You can still set attributes on an event via setAttributes.
You can now explicitly set the timestamp, context, and severity. Severity defaults to INFO=9.

Usage example of the API:

EventLogger eventLogger = eventEmitterProvider.get("test-scoe");

eventLogger
    .builder("namespace.my-event-name")
    // Helper methods to set primitive types
    .put("stringKey", "value")
    .put("longKey", 1)
    .put("doubleKey", 1.0)
    .put("boolKey", true)
    // Helper methods to set primitive array types
    .put("stringArrKey", "value1", "value2")
    .put("longArrKey", 1, 2)
    .put("doubleArrKey", 1.0, 2.0)
    .put("boolArrKey", true, false)
    // Set AnyValue types to encode complex data
    .put(
        "anyValueKey",
        AnyValue.of(
            ImmutableMap.of(
                "childKey1", AnyValue.of("value"),
                "childKey2", AnyValue.of("value"))))
    .emit();

Part of the goal of bundling several breaking changes into a single release to reduce churn.

cc @breedx-splk

jack-berg · 2024-03-21T22:05:59Z

Leaving as a draft PR since this depends on #6316. But wanted to open it up anyway to get eyes on the API ergonomics.

breedx-splk · 2024-03-26T00:06:33Z

This will be quite a bit easier to review after rebasing after #6316, but I'm onboard with this approach.

codecov · 2024-03-26T15:00:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.07%. Comparing base (13ea334) to head (93f6225).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #6318      +/-   ##
============================================
+ Coverage     91.06%   91.07%   +0.01%     
- Complexity     5732     5751      +19     
============================================
  Files           625      626       +1     
  Lines         16748    16782      +34     
  Branches       1713     1718       +5     
============================================
+ Hits          15252    15285      +33     
- Misses         1002     1003       +1     
  Partials        494      494

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

jack-berg · 2024-03-26T15:22:56Z

Ready for review!

scheler · 2024-03-26T16:19:06Z

@jack-berg this API doesn't seem to accommodate setting a primitive data type to the body, which the spec allows.
cc: @breedx-splk

jack-berg · 2024-03-26T16:35:18Z

@jack-berg this API doesn't seem to accommodate setting a primitive data type to the body, which the spec allows.
cc: @breedx-splk

Yup. I figured we'd hold off on that until someone asked for it. Setting the body to something other than a kvlist would look something like EventBuilder set(AnyValue<?> body), and it would override any calls to put(String, ?). Its definitely an advanced / non-standard use case so I'd rather wait until there's a concrete use case.

breedx-splk · 2024-03-26T16:46:48Z

jack-berg#18
Only really mentioning this here because I wanted to explore one idea about what this primitive/string thing might look like in the future. It's not necessary for this PR, but @scheler mentioned it in the SIG call today so I wanted to check it out.

breedx-splk

Yeah this is great! I don't see any reason to hold this up. Let's roll!

I did have a few comments/questions, and I want to make sure that we're able to consider expanding on this in the future to 1) make spec compliant, 2) convenient.

Thanks again for taking all this on. 👍🏻 Much appreciated.

breedx-splk · 2024-03-26T16:53:17Z

  }

  @Override
-  public void emit(String eventName, Attributes attributes) {}


I think it's fine for now, but removing this requires the user wanting to use the "empty" event (where there is no payload, which spec allows) to do eventLogger.builder("foo").build() which is fine, but could have improved ergonomics in the future. 👍🏻

I also respect that we're sensitive to API surface area. 🙃

breedx-splk · 2024-03-26T17:19:12Z

+      values.add(AnyValue.of(val));
+    }
+    return put(key, AnyValue.of(values));
+  }


The test coverage on these default methods comes from the SDK. Do we care about having direct coverage from the API itself?

breedx-splk · 2024-03-26T17:23:59Z

+    if (!hasTimestamp) {
+      logRecordBuilder.setTimestamp(clock.now(), TimeUnit.NANOSECONDS);
+    }
+    logRecordBuilder.setAttribute(EVENT_NAME, eventName);


👍🏻 Solid test coverage on these code paths too....easy to overlook, so thanks!

jack-berg · 2024-03-28T14:50:47Z

Will plan on merging after today's Java SIG if there are no additional comments.

breedx-splk reviewed Mar 26, 2024

View reviewed changes

Comment thread sdk/logs/src/test/java/io/opentelemetry/sdk/logs/internal/SdkEventLoggerProviderTest.java Outdated

breedx-splk reviewed Mar 26, 2024

View reviewed changes

Comment thread sdk/logs/src/test/java/io/opentelemetry/sdk/logs/internal/SdkEventLoggerProviderTest.java Outdated

Refactor Event API to reflect spec changes

d963b68

jack-berg force-pushed the event-payload-log-body branch from dd11124 to d963b68 Compare March 26, 2024 14:33

PR feedback

93f6225

jack-berg marked this pull request as ready for review March 26, 2024 15:22

jack-berg requested a review from a team March 26, 2024 15:22

breedx-splk approved these changes Mar 26, 2024

View reviewed changes

scheler approved these changes Mar 26, 2024

View reviewed changes

breedx-splk mentioned this pull request Mar 28, 2024

Add Event Emitter with Data open-telemetry/opentelemetry-android#122

Closed

jack-berg merged commit 622d977 into open-telemetry:main Mar 29, 2024

tedsuo mentioned this pull request Mar 30, 2024

Describe the use cases for events in greater detail open-telemetry/opentelemetry-specification#3969

Merged

Conversation

jack-berg commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jack-berg commented Mar 21, 2024

Uh oh!

Uh oh!

Uh oh!

breedx-splk commented Mar 26, 2024

Uh oh!

codecov Bot commented Mar 26, 2024

Codecov Report

Uh oh!

jack-berg commented Mar 26, 2024

Uh oh!

scheler commented Mar 26, 2024

Uh oh!

jack-berg commented Mar 26, 2024

Uh oh!

breedx-splk commented Mar 26, 2024

Uh oh!

breedx-splk left a comment

Choose a reason for hiding this comment

Uh oh!

breedx-splk Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

breedx-splk Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breedx-splk Mar 26, 2024

Choose a reason for hiding this comment

Uh oh!

jack-berg commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jack-berg commented Mar 21, 2024 •

edited

Loading