Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.69%. Comparing base(0a2d858) to head(d71743f).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@##           master    #4476      +/-   ##==========================================- Coverage   80.70%   80.69%   -0.02%==========================================  Files         156      156                Lines       16475    16474       -1       Branches     2799     2799              ==========================================- Hits        13296    13293       -3- Misses       2296     2298       +2  Partials      883      883

Q: Was this an actual problem or is this more of a nice-to-have small refactor? As I understand from the other PR, we're actively avoiding having more than one SentryLangchainCallback around at a time.

@sentrivana Yes, so the problem occurred when attempting to reproduce#4443. I was confused why, despite having two callbacks, the data in Sentry was not duplicated. I assumed that perhaps theDedupeIntegration was dropping stuff, but when I enableddebug=True in the SDK init, I got the following error, instead of a notice that theDedupeIntegration dropped the extra span:

 [sentry] ERROR: Internal error in sentry_sdkTraceback (most recent call last):  File "/Users/dszoke/Development/SDK/sentry-python/sentry_sdk/integrations/langchain.py", line 267, in on_llm_end    span_data = self.span_map[run_id]                ~~~~~~~~~~~~~^^^^^^^^KeyError: UUID('2a2c72c8-66f1-4607-b4d2-bd6e93125c39')

The reason, of course, is that the first callback which ends deletes the span from thespan_map, so that when the duplicate callback is called, that span is gone.

Of course, you are right that we try to only have one callback per LLM request in#4485. But, what happens if we have concurrent requests to LLMs, or some other more complex setup, where having two callbacks would be expected? I am unsure how realistic such setups are, but if they do occur, I am pretty confident that we don't want the span map to be shared between the instances.

Not against the change itself, seems reasonable in any case.

I was more pondering if this is behavior that needs testing (rather than a small internal refactor), but if you believe it is, I'd saytest_span_map_is_instance_variable is sufficient. I'd removetest_span_map_not_class_attribute as it doesn't add any value -- we should test behavior, not implementation details, and the behavior in this case is covered by the first test already.

sounds reasonable, the first test is definitely more important anyhow