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

Introduce new semantic conventions for CloudEvents #1978

Merged
merged 38 commits into from
Mar 10, 2022

Conversation

joaopgrassi
Copy link
Member

@joaopgrassi joaopgrassi commented Sep 29, 2021

Closes #1951, Resolves #1949

Changes

This PR is a follow-up of PR #1951.

In that PR, the CloudEvents semantic conventions were added together with the messaging ones. After the messaging WG meeting on 23.09.2021 (meeting notes), it was agreed that the attributes introduced there make sense, but they should be outside of the messaging semantic conventions, to avoid confusion of CloudEvents being a messaging system.

This spec adds guidance on how/when to create Spans for CloudEvents, how context should be propagated and lists attributes that can be added to such Spans.

@lmolkova
Copy link
Contributor

Since CloudEvents spec defines distributed tracing extension, OTel conventions should cover how it's populated.

And then it would still collide with WIP messaging spec.

I suggest we either figure it out first or explicitly mention in this spec that instrumentation SHOULD NOT populate cloud event distributed tracing context until we know how it should work.

@joaopgrassi
Copy link
Member Author

@lmolkova I'm adding instrumentation to a project that uses the CloudEvents Go SDK and in order to get "proper" traces, I inject the tracecontext into the event and I used the distributing tracing extension for that: https://github.com/dynatrace-oss-contrib/sdk-go/blob/add-otel-obsservice/observability/opentelemetry/v2/client/cloudevents_carrier.go.

I understand your point, but until a proper solution is in place, what should users do? Is my solution there completely discouraged?

@lmolkova
Copy link
Contributor

@lmolkova I'm adding instrumentation to a project that uses the CloudEvents Go SDK and in order to get "proper" traces, I inject the tracecontext into the event and I used the distributing tracing extension for that: https://github.com/dynatrace-oss-contrib/sdk-go/blob/add-otel-obsservice/observability/opentelemetry/v2/client/cloudevents_carrier.go.

I understand your point, but until a proper solution is in place, what should users do? Is my solution there completely discouraged?

I work on the SDKs which do the same, so I feel your pain. I think our SDKs should keep doing what they are doing if it helps users, but it's not ready to be an official spec - we should be ready for how this context is propagated to change (or maybe a priority of this context to change when cloud event is transferred over something that supports context propagation too, e.g. AMQP).

Maybe there is some middle ground where we say that you MAY propagate context and here's how, but the details are still in the works and may change?

@joaopgrassi
Copy link
Member Author

Maybe there is some middle ground where we say that you MAY propagate context and here's how, but the details are still in the works and may change?

Yes that seems reasonable. The "POC" I did with CloudEvents in Go injecting/extracting using the distributed tracing extension worked well I think, but definitely having something "built-in" is the best in the long run.

From my point maybe it's not so bad that things could change later. The PR I sent there for CloudEvents offers the inject/extract but you don't need to use it, if you are sending the events via HTTP and all is auto-instrumented then things will work. But for my case, it was necessary and I need it "today". So, until we have a clear path going forward, having a middle ground solution like you said makes sense. Me as a user can benefit from OTel today. If later I just update the SDK and things work as well, even better.

@pyohannes
Copy link
Contributor

Since CloudEvents spec defines distributed tracing extension, OTel conventions should cover how it's populated.

I submitted a separate issue for this: open-telemetry/semantic-conventions#654

We will discuss this when we discuss context propagation in relation to the messaging semantic conventions. At this point we can't define how we populate it, as we don't know how to use it (and whether it will be of use at all).

In this way the context propagation question shouldn't block this PR, but it can be amended later if we come to a conclusion on this.

@arminru arminru added area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory labels Sep 30, 2021
@Oberon00
Copy link
Member

@lmolkova

And then it would still collide with WIP messaging spec.

I suggest we either figure it out first or explicitly mention in this spec that instrumentation SHOULD NOT populate cloud event distributed tracing context until we know how it should work.

I think @joaopgrassi took part in a messaging SIG meeting and the outcome was that CloudEvents are not really messaging. I would agree to that.

CloudEvents IMHO might be sent over a messaging system, but they might be sent over HTTP or stored in a database or be only used in-process as well. So IMHO, CloudEvents are a concept in their own right, not tied to any other semantic convention.

@lmolkova
Copy link
Contributor

lmolkova commented Sep 30, 2021

To create a spec that fits other SDKs that do CloudEvents, let me share a few examples here

https://docs.microsoft.com/en-us/java/api/overview/azure/messaging-eventgrid-readme?view=azure-java-stable#sending-cloudevent-to-a-topic-that-accepts-cloudevent-schema

List<CloudEvent> events = new ArrayList<>();
User user = new User("John", "James");
events.add(new CloudEvent("https://source.example.com", "Com.Example.ExampleEventType",
    BinaryData.fromObject(user), CloudEventDataFormat.JSON, "application/json"));
cloudEventClient.sendEvents(events);

The important part here is that you can send cloud events over other things, i.e. multiple clients accept a batch of events. (and same clients can send other things)

I can't really speak on behalf of AWS, but AWS EventBridge or CloudWatch do something similar (AFAIK using CloudEvents at least something similar that has a bridge to them) https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/examples-cloudwatch-send-events.html

PutEventsRequestEntry request_entry = new PutEventsRequestEntry()
    .withDetail(EVENT_DETAILS)
    .withDetailType("sampleSubmitted")
    .withResources(resource_arn)
    .withSource("aws-sdk-java-cloudwatch-example");

PutEventsRequest request = new PutEventsRequest()
    .withEntries(request_entry);

PutEventsResult response = cwe.putEvents(request);

So

  • creation is separate from publishing,
  • batching is common
  • send span can be related to message but does not have to (I can buffer up events and send later).
  • transport and client SDKs are independent of the cloud events creation and consumption

I'll leave a few comments on the spec to make it work for Azure EventGrid with CloudEvents

Copy link
Contributor

@lmolkova lmolkova left a comment

Choose a reason for hiding this comment

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

I would still add a remark on context propagation, saying that it's being defined and you MAY use cloud events extension, but if you send cloud events over some messaging system covered by OTel messaging conventions, you SHOULD follow messaging conventions instead.

specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 4, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Feb 4, 2022
@joaopgrassi
Copy link
Member Author

Hi @tigrannajaryan, just pinging to see if you had a chance to see the replies.

@github-actions github-actions bot removed the Stale label Feb 8, 2022
@joaopgrassi
Copy link
Member Author

Hi @pyohannes, @lmolkova, @arminru, I updated the PR a bit, according to this conversation I had with @tigrannajaryan (#1978 (comment)). Could you please take another look?

The changes are around explaining the context propagation portion and adding some example diagram/traces to clarify why this is necessary. Let me know your thoughts.

@github-actions
Copy link

github-actions bot commented Mar 5, 2022

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions bot added the Stale label Mar 5, 2022
Copy link
Member

@arminru arminru left a comment

Choose a reason for hiding this comment

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

Thanks for adding additional context and explanations. LGTM!

specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
specification/trace/semantic_conventions/cloudevents.md Outdated Show resolved Hide resolved
semantic_conventions/trace/cloudevents.yaml Outdated Show resolved Hide resolved
@arminru arminru merged commit 37919a6 into open-telemetry:main Mar 10, 2022
@arminru arminru deleted the semconv-cloudevents branch March 10, 2022 18:11
@joaopgrassi
Copy link
Member Author

Thanks all for the help with the reviews! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:semantic-conventions Related to semantic conventions spec:trace Related to the specification/trace directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add semantic conventions for CloudEvents
8 participants