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

Just riffing to see what this _might_ look like. #18

Conversation

breedx-splk
Copy link

I primarily wanted to see how interesting/painful it might be to set the payload to an arbitrary value or primitive (eg. AnyValue.of(Long) or whatever. This was motivated by @scheler pointing out that open-telemetry#6813 doesn't quite match the language in the spec, and some users might have a need for this (especially in the case of String).

Mostly offering this here for discussion. @jack-berg has already responded in open-telemetry#6813 that he'd like to add this on later, so we can definitely defer until later or until it's specifically asked for.

At least trying this out helped me to understand that adding it on later is straightforward given the API implementation in open-telemetry#6813, and that we weren't making it too difficult.

Comment on lines +40 to +41
this.payload.clear();
((ExtendedLogRecordBuilder) logRecordBuilder).setBody(payload);
Copy link
Author

Choose a reason for hiding this comment

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

This might be the grossest part of this. It's also a little weird of an API because the builder could get reused and it's a bit of a chimera -- handling both simple types and complex types. Like, a confused user might call put(), put(), put() then setPayload() then put(), put(), put() again.

Not sure why you'd ever want to or why one might think that should work, but it's possible. Just wanted to point it out.

Copy link
Owner

Choose a reason for hiding this comment

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

handling both simple types and complex types. Like, a confused user might call put(), put(), put() then setPayload() then put(), put(), put() again.

Yeah in my head we'd solve this by having a nullable reference to the user set AnyValue payload. And then down in emit, if explicit payload was set, it takes priority over any calls to put. Something like:

@Nullable private AnyValue<?> explicitPayload;

...

public EventBuilder setPayload(AnyValue<?> payload) {
  this.explicitPayload = payloadl;
  return this;
}

...

@Override
public void emit() {
  if (explicitPayload != null) {
     ((ExtendedLogRecordBuilder) logRecordBuilder).setBody(explicitPayload);
  } else if (!payload.isEmpty()) {
    ((ExtendedLogRecordBuilder) logRecordBuilder).setBody(AnyValue.of(payload));
  }
  if (!hasTimestamp) {
    logRecordBuilder.setTimestamp(clock.now(), TimeUnit.NANOSECONDS);
  }
  logRecordBuilder.setAttribute(EVENT_NAME, eventName);
  logRecordBuilder.emit();
}

Potential to cause confusion to users, but can mitigate with javadoc explaining the behavior. You might say that this might be cause to remove / change the put(String, ?) methods, but I think that those are absolutely crucial from an ergonomics perspective.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah that's cool, the null was another approach that occurred to me, and I think the usability is fine and maybe slightly cleaner impl. I agree that those put() calls cannot go away!

@breedx-splk
Copy link
Author

I think the API has moved well beyond this now. 🍻

@breedx-splk breedx-splk closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants