search

census-instrumentation / opencensus-python

A stats collection and distributed tracing framework
Apache License 2.0
669 stars 250 forks source link

Fixed AzureLogHandler with multiple processes. #1158

Open JeremyVriens opened 2 years ago

JeremyVriens commented 2 years ago

These changes fix both https://github.com/census-instrumentation/opencensus-python/issues/900 and https://github.com/census-instrumentation/opencensus-python/issues/928, but I'm not so sure about the compatibility with Python2. I changed the normal queue.Queue to a multiprocessing.Queue and converted the LogRecord to an envelope before putting it on the queue as serializing a LogRecord didn't work when it contained a traceback.

google-cla[bot] commented 2 years ago

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

lzchen commented 2 years ago

@JeremyVriens Thanks for the contribution! Please sign the CLA when you have the time.

JeremyVriens commented 2 years ago

@JeremyVriens Thanks for the contribution! Please sign the CLA when you have the time.

I did, but I did a commit with the wrong git email config. Will try to fix this asap when I got time.

JeremyVriens commented 2 years ago

This is a hard to tackle issue (you can tell by the bug report discussions) and I'm not so sure if there's a different/better way of sharing the queue between different processes. I do agree that the performance implication is not desired, but I think it's inevitable when you work with different processes. I will spend some more time to see if I can make it configurable (perhaps an option where you can say: multiprocessing=True/False(default)) and at least for now this is a working PoC.

Korred commented 2 years ago

@JeremyVriens thanks for your POC! I just came across issue #900 a few days ago, while trying to pass logs from Celery to Azure Application Insights and seeing that this was first reported 2 years ago I actually didn't expect this issue to be picked up and "fixed" in the near future. Getting this properly fixed would be really helpful for a lot of people with this kind of setup!

mockodin commented 1 year ago

This works great for monkey patching.. any plans to complete this?

I don't believe python2 should be a concern at this point.

@JeremyVriens is looks like this is largely just pending CLA check for you. Alternatively I suppose someone could resubmit if you are no longer watching this?

burkol commented 1 year ago

Hi All,

I faced the same issue. Is there a possibility that it will be accepted in the near future?

Regards, Zoltan

vvxhid commented 1 year ago

May I ask for an estimated merge date for this pull request? Thanks!