Update addEvent API to attach events to span in Context #22332

samvaity · 2021-06-16T22:53:59Z

simplynaveen20 · 2021-06-17T18:53:14Z

...ng-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryTracer.java

@@ -199,11 +199,20 @@ public Context getSharedSpanBuilder(String spanName, Context context) {
     */
    @Override
    public void addEvent(String eventName, Map<String, Object> traceEventAttributes, OffsetDateTime timestamp) {
+        addEvent(eventName, traceEventAttributes, timestamp, Context.NONE);


Why we need this api , Context.NONE will not have the span info ?

Since this is an existing API and we do not want to break any existing users using this API by removing it.

Can we do deprecation,document update because this method will return without doing anything, so could be misleading.

BTW, it doesn't look like there are any users https://github.com/Azure/azure-sdk-for-java/search?q=addevent. are we expecting anyone outside of this repo to use it?

I understand it's a public API and can't be simply removed. Agree that deprecation would be great.
Just curious if we expect anyone else to ever use it :)

are we expecting anyone outside of this repo to use it?

Anyone using their own implementation of the azure-core, Tracer class would/could be using this API.
Also, we sort to deprecation as the last resort and if use any workarounds instead according to our guidance here
https://github.com/Azure/azure-sdk-for-java/wiki/Deprecation

yeah, I'm checking my understanding: if something calls into addEvent (which would be one of the Azure client libraries), then custom Tracer implementation would override it. But nothing calls it yet, correct?

is this needed to maintain existing behavior? (agree hopefully no one is using it anyways)

Suggested change

addEvent(eventName, traceEventAttributes, timestamp, Context.NONE);

addEvent(eventName, traceEventAttributes, timestamp, new Context(PARENT_SPAN_KEY, Span.current()));

Agree with @trask, I think it would make sense to keep this overload with a defaulting behavior to look for the current span in scope. For the users, who do not want any specific span for the event to be attached to.
@lmolkova thoughts?

(I would also support deprecation, this method does seem a bit out of place since the azure tracing API seems generally designed around passing explicit context)

samvaity · 2021-06-17T18:55:49Z

/azp run java - core - ci

azure-pipelines · 2021-06-17T18:56:05Z

Azure Pipelines successfully started running 1 pipeline(s).

alzimmermsft · 2021-06-17T20:09:51Z

...ng-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryTracer.java

        if (currentSpan == null) {
-            logger.info("Failed to find a starting span to associate the %s with.", eventName);
+            logger.info("Failed to find a starting span to associate the event %s with.", eventName);


Not related, but do we think this log message should be VERBOSE level?

check-enforcer · 2021-06-23T00:41:32Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run java - [service] - ci

srnagar · 2021-06-23T17:37:26Z

sdk/core/azure-core/src/main/java/com/azure/core/util/tracing/Tracer.java

@@ -251,4 +251,23 @@ default Context getSharedSpanBuilder(String spanName, Context context) {
     */
    default void addEvent(String name, Map<String, Object> attributes, OffsetDateTime timestamp) {


Are we not deprecating this as @trask suggested? I agree that this interface design is centered around passing the Context to each method. So, this method should be deprecated and still use the default behavior to use the current span if someone calls it.

srnagar · 2021-06-23T19:27:00Z

...ng-opentelemetry/src/main/java/com/azure/core/tracing/opentelemetry/OpenTelemetryTracer.java

        Objects.requireNonNull(eventName, "'eventName' cannot be null.");

-        Span currentSpan = Span.current();
+        Span currentSpan = getOrDefault(context, PARENT_SPAN_KEY, null, Span.class);
        if (currentSpan == null) {
            logger.info("Failed to find a starting span to associate the {} with.", eventName);


could this be logged at verbose level?

ghost added the Azure.Core azure-core label Jun 16, 2021

samvaity self-assigned this Jun 17, 2021

simplynaveen20 reviewed Jun 17, 2021

View reviewed changes

samvaity marked this pull request as ready for review June 17, 2021 18:55

samvaity requested review from alzimmermsft, anuchandy, hemanttanwar, jianghaolu, lmolkova, srnagar and trask as code owners June 17, 2021 18:55

alzimmermsft reviewed Jun 17, 2021

View reviewed changes

alzimmermsft approved these changes Jun 17, 2021

View reviewed changes

srnagar reviewed Jun 23, 2021

View reviewed changes

samvaity added 2 commits June 23, 2021 10:56

add addevent overload

778fdbe

add defaulting behavior for addEvent overload

d053f57

samvaity force-pushed the fix-22115 branch from 5402c76 to d053f57 Compare June 23, 2021 17:58

update to deprecated

3a229a2

srnagar approved these changes Jun 23, 2021

View reviewed changes

add deprecation suppression

c77772b

samvaity merged commit 16c2d5a into Azure:main Jun 23, 2021

samvaity deleted the fix-22115 branch July 17, 2021 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update addEvent API to attach events to span in Context #22332

Update addEvent API to attach events to span in Context #22332

samvaity commented Jun 16, 2021 •

edited

Loading

simplynaveen20 Jun 17, 2021

samvaity Jun 17, 2021

simplynaveen20 Jun 17, 2021

lmolkova Jun 17, 2021

samvaity Jun 17, 2021

lmolkova Jun 17, 2021 •

edited

Loading

trask Jun 18, 2021

samvaity Jun 18, 2021

trask Jun 18, 2021

samvaity commented Jun 17, 2021

azure-pipelines bot commented Jun 17, 2021

alzimmermsft Jun 17, 2021

check-enforcer bot commented Jun 23, 2021

srnagar Jun 23, 2021

srnagar Jun 23, 2021

	addEvent(eventName, traceEventAttributes, timestamp, Context.NONE);
	addEvent(eventName, traceEventAttributes, timestamp, new Context(PARENT_SPAN_KEY, Span.current()));

		@@ -251,4 +251,23 @@ default Context getSharedSpanBuilder(String spanName, Context context) {
		*/
		default void addEvent(String name, Map<String, Object> attributes, OffsetDateTime timestamp) {

Update addEvent API to attach events to span in Context #22332

Update addEvent API to attach events to span in Context #22332

Conversation

samvaity commented Jun 16, 2021 • edited Loading

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

lmolkova Jun 17, 2021 • edited Loading

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

samvaity commented Jun 17, 2021

azure-pipelines bot commented Jun 17, 2021

Choose a reason for hiding this comment

check-enforcer bot commented Jun 23, 2021

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samvaity commented Jun 16, 2021 •

edited

Loading

lmolkova Jun 17, 2021 •

edited

Loading