Skip to content

Flush signals after initializing exporters #839

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deividasstr
Copy link
Contributor

@deividasstr deividasstr commented Feb 19, 2025

Based on issue, flushing existing events on sdk initialization to increase chances signals are sent in case app is terminated before export interval is reached.

Flushes all exporters during SDK build, in async task of initiating exporters.

Maybe there is a better place and time to do it? Sometime after build?

If location is suitable, I will add tests.

@deividasstr deividasstr requested a review from a team as a code owner February 19, 2025 10:02
@breedx-splk
Copy link
Contributor

I am not sure I follow this. Flushing right after creation? There seems to still be a window of time after the flush and before the export interval is reached.

There's a shutdown hook in the upstream sdk that is intended to handle this case, I think.

@deividasstr
Copy link
Contributor Author

I am not sure I follow this. Flushing right after creation? There seems to still be a window of time after the flush and before the export interval is reached.

It sounds like this comment is based on second comment. But if the latter does not work, the event remains unexported until first export interval.

There's a shutdown hook in the upstream sdk that is intended to handle this case, I think.

As @LikeTheSalad said in the issue, flushing on app closing is not reliable, the export is not ensured.

Actually I did notice there is an automatic shutdown hook. But from my testing, I have never seen an export happening on app being killed. I will take a look at the flow

@LikeTheSalad
Copy link
Contributor

I think it's also worth noticing that there are 2 types of "flushing" in this repo, one to disk and the other to the network. Definitely the network one isn't reliable on app destroy, probably the disk one is, although I'm not sure because I haven't tested it.

@deividasstr
Copy link
Contributor Author

I think it's also worth noticing that there are 2 types of "flushing" in this repo, one to disk and the other to the network. Definitely the network one isn't reliable on app destroy, probably the disk one is, although I'm not sure because I haven't tested it.

Aren't both of them attempted on flush of the reader?

@deividasstr
Copy link
Contributor Author

There's a shutdown hook in the upstream sdk that is intended to handle this case, I think.

From my lookup, due to long chain of dependencies, I do not see where AutoConfiguredOpenTelemetrySdkBuilder is used in android SDK. And the OpenTelemetrySdk itself does also have shutdown method, which is called on close method on implementation of Closeable. But I do not see this being attached to any shutdown or use anywhere.

So it is not used, right?

@breedx-splk
Copy link
Contributor

Hey @deividasstr sorry for the delay in circling back on your PR. It is appreciated.

From my lookup, due to long chain of dependencies, I do not see where AutoConfiguredOpenTelemetrySdkBuilder is used in android SDK.

That's correct -- we don't use the AutoConfiguredOpenTelemetrySdkBuilder on Android because much of the "auto" in the autoconfiguration is based on environment variables and system properties -- the use of which is limited/nil on Android. We could maybe see some benefit from the service loader bits, but then we have a kind of split-config mode, and it's probably more confusing than it's worth.

And the OpenTelemetrySdk itself does also have shutdown method, which is called on close method on implementation of Closeable. But I do not see this being attached to any shutdown or use anywhere.

Oh crap, ok, I think you're right. I'm sorry for misleading comments earlier, I think I was mistaken!

So it is not used, right?

Yeah, I think this is missing right now, and we need something to flush data at exit so that we don't lose it. Fortunately, with disk buffering stuff should leave memory pretty quickly, but it probably should be made more robust.

Sorry to continue this conversation in the PR here -- we should be chatting more in the issue. Regardless, I'm not sure why this PR is doing a flush after exporter creation. The setDelegate() does a flush internally, so any telemetry buffered at startup before the exporters are initialized (async, concurrently) should be safely flushed.

Did you have another use case in mind with this? If you're agreeable to the idea, I would prefer to close this one and see if someone (you?) could build the flush-on-shutdown version instead?

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.

3 participants