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

Add ForceFlush to Tracing SDK #1452

Merged

Conversation

kipwoker
Copy link
Contributor

@kipwoker kipwoker commented Feb 19, 2021

Fixes #1451

Changes

Added ForceFlush to Tracing SDK like it was done for the Shutdown method.

@kipwoker kipwoker marked this pull request as ready for review February 19, 2021 10:07
@kipwoker kipwoker requested review from a team as code owners February 19, 2021 10:07
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

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

Thank you!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
@carlosalberto
Copy link
Contributor

Overall good, but I suggest we wait for next Tuesday to discuss it (given the historical fear of adding this kind of functionality).

@yurishkuro
Copy link
Member

Observation: despite being in the SDK spec, this PR proposes a change to TraceProvider, which is a part of the API, and it is a breaking change (adding methods to interface breaks existing implementations of that interface). While it is fine to add new methods to concrete SDK classes implementing TraceProvider, this change is very easy to misinterpret. Also, it is not clear how the end user is supposed to interact with this functionality - the flush won't be available at the API level.

@Oberon00
Copy link
Member

Oberon00 commented Feb 19, 2021

Observation: despite being in the SDK spec, this PR proposes a change to TraceProvider, which is a part of the API

No, it does not. It proposes to add this to the SDK implementation class of the TracerProvider. The same applies to Shutdown.

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Thank you!

@reyang reyang added area:sdk Related to the SDK spec:trace Related to the specification/trace directory labels Feb 19, 2021
@bogdandrutu
Copy link
Member

Observation: despite being in the SDK spec, this PR proposes a change to TraceProvider, which is a part of the API

This is a public api for the SDK implementation of the TraceProvider, but it is not an "interface" and even if it is we do not expect others to implement the SDK interfaces that we have (only for the API we guarantee that).

bogdandrutu and others added 2 commits February 19, 2021 11:39
Co-authored-by: Christian Neumüller <christian+github@neumueller.me>
Co-authored-by: Bogdan Drutu <lazy@splunk.com>
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

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

+1. We already recommend ForceFlush for ephemeral situations like CloudRun.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

@yurishkuro do you still have a concern?

@tigrannajaryan
Copy link
Member

Just to make it clear: interfaces that the SDK must implement (such as TraceProvider interface) are NOT part of our compatibility guarantees from the perspective of the implementor of the SDK. The interface may change in such a way that in turn requires the SDK code to change. Our guarantees say that the interface cannot change in a way that the requires end-user code to change.

Interfaces that define the interaction between the API and SDK can be modified by us. There is no expectation that the end user implements a TraceProvider and then expects that the TraceProvider interface remains stable in such a way that their implementation will not be broken over time.

Note 1: we have a requirement that newer versions of the SDK package must work correctly with older versions of the API packages, so we cannot do arbitrary changes to the interfaces between API and SDK. From the perspective of the API the callable interface of the SDK is guaranteed to be backwards compatible.

Likewise SDK's interfaces that are intended to be called by the end user have the exact same requirement: the may evolve in a way that remains callable with the existing end-user code without changing anything in the end-user code.

Adding ForceFlush satisfies all above requirements. It is part of TraceProvider which is callable by the end user or by the API package, but it is not intended to be implemented by the end user.

Note 2: interfaces that are intended to be implemented by the end user (for example Sampler) have a different, stricter requirement. We must not change them in a way that breaks existing end user code that implements the interfaces. Such interfaces are considered to be part of our contract with the end user and are an integral part of our API.

@Oberon00
Copy link
Member

Oberon00 commented Feb 23, 2021

Note 2: interfaces that are intended to be implemented by the end user (for example Sampler) have a different, stricter requirement. We must not change them in a way that breaks existing end user code that implements the interfaces. Such interfaces are considered to be part of our contract with the end user and are an integral part of our API.

@bogdandrutu and I wrote a few words about that on #1454. Since comments on a merged PR are hard to notice but I think this is an important topic, I will open a new issue for that. EDIT: I created #1457 "Interface stability for implementers"

@kipwoker kipwoker deleted the kipwoker/forceflush-tracerprovider branch February 23, 2021 08:06
@Oberon00
Copy link
Member

@tigrannajaryan also note that this PR does not even modify the TracerProvider interface, it modifies the SdkTracerProvider implementation class.

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

Successfully merging this pull request may close these issues.

Add ForceFlush to Tracing SDK