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

[trytond] Tracing and transaction info #2652

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

n1ngu
Copy link
Contributor

@n1ngu n1ngu commented Jan 17, 2024

Add tracing to the Trytond integration and extract transaction information from RPC request objects.

@n1ngu
Copy link
Contributor Author

n1ngu commented Jan 17, 2024

Any insight on how to test this is very welcome

This unblocks the roead for setting sentry up as trytond middleware
(otherwise this integration would trigger circular import errors)
https://docs.tryton.org/projects/server/en/latest/topics/configuration.html#wsgi-middleware
@n1ngu n1ngu force-pushed the trytond/tracing branch 2 times, most recently from 48fa779 to 823c31e Compare January 17, 2024 15:18
tox.ini Show resolved Hide resolved
tox.ini Show resolved Hide resolved
@n1ngu
Copy link
Contributor Author

n1ngu commented Jan 17, 2024

@pokoli sorry to bother you. Just pushed the tracing feature commit with a fair amount of metaprogramming black magic. I am still figuring out some typing annotation details but I'd enjoy your review on that part.

It is working just fine in some production deployments but I may have missed some thoughts for multi-tenancy deployments.

imatge

Due to the dynamic nature of Tryton's ORM, classes and methods can't be
statically instrumented with Sentry's "functions_to_trace" parameter.
Instead, add mixins that will instrument framework most relevant methods
in the Pool post-setup phase.
@pokoli
Copy link

pokoli commented Jan 18, 2024

@n1ngu starting from 6.8 series there is a new log feature, which is used by the tryton server to log events from the user.
Maybe you will like to trace such events also in sentry. Do you think this is doable?

@n1ngu
Copy link
Contributor Author

n1ngu commented Jan 18, 2024

@n1ngu starting from 6.8 series there is a new log feature, which is used by the tryton server to log events from the user. Maybe you will like to trace such events also in sentry. Do you think this is doable?

Because that logging is implemented with a ModelStorage, any "ir.model.log":create call will get its own span with the present PR! Model.log could be instrumented just as create, copy, delete, etc, but there is very few code there. I think it would be more interesting to create spans for Transaction.commit or Transaction._store_log_records but that can be done out of the box with

sentry_sdk.init(
    traces_sample_rate=1/128,
    functions_to_trace=[
        {"qualified_name": "trytond.transaction.Transaction.commit"},
        {"qualified_name": "trytond.transaction.Transaction._store_log_records"},
    ],
)

because, unlike pool models, Transaction can be statically instrumented. Transaction.start is explicitly patched in this PR because I find the whole context manager it creates deserves a span but couldn't figure out how to do that with functions_to_trace (I feel I am missing a more obvious solution here)

I don't know if you meant to capture log events in sentry with capture_message to send events irrespective of unhandled exceptions and tracing sample rate? That'd be certainly interesting and easily doable! I'll give the idea a try in my staging deployments while this PR settles. I feel the major challenge will be configuring this not to wipe your sentry event ingestion quota 🥲

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.

None yet

2 participants