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

bugfix: batch processor doesn't work with uwsgi #2277

Merged
merged 7 commits into from
Jun 8, 2022

Conversation

srikanthccv
Copy link
Member

Description

The change introduced in #2242 didn't work for uwsgi since its worker process management is implemented in C and _at_fork_reinit is never invoked. We solve this by invoking it manually when we find process ids do not match.

Fixes #2266

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

@Cruuncher
Copy link

I need to use opentelemetry with uwsgi.

Are there plans for this to release? Can I work around this by monkey patching in this implementation of BatchSpanProcessor?

@srikanthccv
Copy link
Member Author

Hi @Cruuncher ,

We have documented here how to use the uwsgi hooks here. I didn't get the time to test this change. I will revisit it this weekend.

@Cruuncher
Copy link

Cruuncher commented Mar 2, 2022

@srikanthccv Thank you you for pointing me to the relevant documentation! I've been really spinning my wheels trying to get this working, and googling doesn't really find the answers 😓

This appears to be working for us now

@srikanthccv srikanthccv marked this pull request as ready for review June 1, 2022 21:07
@srikanthccv srikanthccv requested a review from a team June 1, 2022 21:07
@srikanthccv
Copy link
Member Author

cc @aabmass

@ocelotl ocelotl added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jun 7, 2022
@lzchen
Copy link
Contributor

lzchen commented Jun 7, 2022

This fix would probably solve all cases in which os.register_at_fork isn't available when a new process is created correct? After this change, I think the only case we are missing is when os.register_at_fork is not available (like in Windows) and when a new process is not created (simply a new fork is created).

@srikanthccv
Copy link
Member Author

and when a new process is not created (simply a new fork is created).

When does this happen?

One thing I am still thinking about is how do I do the same for PeriodicExportingMetricReader? Unlike tracing/logging where the events are received by processor, the PMR collects them with collect function. There is nothing like emit or on_start/on_end where I can check if the pids to see the fork hook is invoked and invoke if not.

@lzchen
Copy link
Contributor

lzchen commented Jun 7, 2022

When does this happen?

Oh I guess it wouldn't happen for the cases we care about (only when child processes are spawned).

Different question: Is there still a need for os.register_at_fork if we can compare pids all the time?

@srikanthccv
Copy link
Member Author

Is there still a need for os.register_at_fork if we can compare pids all the time?

It's fine to still have it. With the hook the reinit happens immediately but with pids records need to arrive at emit/on_end then it invokes the reinit (this code path very less likely compared to fork hook).

@lzchen
Copy link
Contributor

lzchen commented Jun 7, 2022

One thing I am still thinking about is how do I do the same for PeriodicExportingMetricReader?

I am okay to discuss this in a separate issue/pr to unblock this.

@lzchen lzchen merged commit 9fbc93b into open-telemetry:main Jun 8, 2022
@srikanthccv srikanthccv deleted the bugfix-issue-2266 branch June 8, 2022 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Batch Span Processor does not work with uwsgi
4 participants