[8.18.1] warn_stacklevel is slowing down the client

[folkvir]

Hello!

We noticed in the 8.18.1 client that the function warn_stacklevel called when a warning is emitted that the function used to fetch the stack is slowing down our service with at least a factor 2. As an example, for a normal 45/55 minutes run of tests with the client 7 we now waits for almost 2 hours with the client 8 for the same workload and same number of tests.

Compared to the client v7 where the stacklevel was 2, here you dynamically fetch the stacklevel by parsing the stack through inspect.stack() which can be very very slow.

Here an example of calling 2 times get_alias with the client v8 instrumented by pyinstrument.

As you can see the warn_stacklevel is very time consuming compared to the request itself.

As a temporary patch we fixed this value to 0 but this is not a long-term solution so if you could provide a better alternative it will be very appreciated!

[miguelgrinberg]

The warn_stacklevel function only runs when there are warnings to report. This function is inefficient because it looks through the call stack to find what is a meaningful source file and line number to report that can help you address the warning. A simpler implementation would be less useful, since it would not give you the correct location from where the warning was triggered.

The point of reporting these warnings is that you address them. Why are you getting so many warnings, to the point that they are slowing down your application? To me it seems the best solution is for you to address these warnings, but I think it would be a good idea for us to provide an option to disable warning reports, or more accurately, to not run this function when the warnings are externally disabled.

[folkvir]

We are currently in a migration process and we live with a lot of deprecation warnings. While I agree with you on the fact that we should address them as soon as possible to avoid issues, we also need to live with some warnings for certain reasons like the time to address the warnings, continuous deployment, etc... The service needs to continue without slowdown.

My point is that with the version 7, deprecation warnings were not a performance issue because the stacklevel was a fixed value. We expected to also be the case with version 8 of the client. Those deprecation warnings are very important and I don't say to remove them but rather disable the dynamic stacklevel search in a production environment.

A solution would be to allow the stacklevel to be an option with a default value returned by the warn_stacklevel() function.
Something like: stacklevel: Optional[int] = None and stacklevel=config.stacklevel if config.stacklevel is not None else warn_stacklevel()

[miguelgrinberg]

@folkvir The level of deprecation notices in the 7.x code base was minimal. I wasn't involved with Elastic back when this change was made, but I can see how things got much more complicated after the 8.0 release. There are several more places in the code that issue warnings, and the stack level to report the correct file and line number is not the same in all of them. You seem to imply there is one place where this is changed and that's it, but that is not the case, from a quick code search I counted 9 different places in the client that emit warnings, and depending on the stack traces even from a single location the stack level may need to be different depending on the particular case and what the call stack looks at the time the warning needs to be issued.

In case this isn't clear, if the stack level is not set correctly, then the file and line number that will be reported with the warning are not going to be the location in your code where the warning was triggered, but some other location above or below in the call stack. Using a constant stack level for all warnings will cause some to show a file and line that are off.

What I think we could do, as I said above, is disable the stack level calculation with an option. That will mean that all warnings that you receive will come without any useful file and line information.

[folkvir]

You seem to imply there is one place where this is changed and that's it, but that is not the case, from a quick code search I counted 9 different places in the client that emit warnings

This was just a first suggestion, I did a quick search in the codebase but I completely agree that warnings could be emitted elsewhere without any context of the client configuration.

The solution you're thinking of is what we are looking for, aka continuing to emit the warnings but without any mention of the location (file and line information) of the warning. 👍 Thank you for your reactivity by the way!