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

Ownership integration #1364

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Ownership integration #1364

wants to merge 4 commits into from

Conversation

unflxw
Copy link
Contributor

@unflxw unflxw commented Jan 9, 2025

Implements an integration for the ownership gem, as discussed on Slack.

Commits

Add instrument_ownership config option

Add a new instrument_ownership configuration option that can be
used to toggle off instrumentation for the ownership gem.

Add after_create transaction hook

Add a hook that allows for blocks of arbitrary code to be applied to
modify a transaction after it has been created.

Add before_complete transaction hook

Add a before_complete transaction hook, which is executed right
before a transaction is completed. Note that, for transactions
containing multiple errors, this hook is called once for each of
the duplicate transactions that is used to report each of the
errors. This allows the context to be customised for each error.

Instrument ownership gem

Add an instrumentation for the ownership gem. When a transaction
is created, if an owner has been set in the gem, use the name of
the owner as the namespace for the transaction. Do the same when an
owner is set during an active transaction.

If more than one owner is set during a transaction, the namespace
will be that of the last owner that was set.

Notes

@thijsc mentioned it would be nice to contribute this to the gem itself. I took a look at that, but I noticed we'd need something like the after_create and before_complete hooks in this PR to make it work. That said, now that those hooks exist, it should be possible to make them part of the public API, discard the instrumentation in this PR, and instead contribute an instrumentation as a PR to the gem.

@matsimitsu suggested using the owner information to automatically assign errors (and performance issues?) to teams, instead of setting the owner as the namespace. At the moment, this PR uses namespaces, but we could use team assignments instead, or alongside it. Beyond this gem, customers might find it useful to be able to set metadata on their transactions and traces that causes them to automatically be assigned to a team or person. I'm unsure how much work that would be, though.

Regarding testing it, I attempted to use a mock to test it, like we do for several other dependencies. It is possible, but I found myself re-implementing the gem and doubting whether my implementation was an accurate representation of the original, so I decided to add a gemfile instead. I'm happy to reconsider this if we want to avoid the additional gemfile and its corresponding CI runs.

@backlog-helper
Copy link

backlog-helper bot commented Jan 9, 2025

✔️ All good!

New issue guide | Backlog management | Rules | Feedback

unflxw added 3 commits January 9, 2025 17:08
Add a new `instrument_ownership` configuration option that can be
used to toggle off instrumentation for the `ownership` gem.
Add a hook that allows for blocks of arbitrary code to be applied to
modify a transaction after it has been created.
Add a `before_complete` transaction hook, which is executed right
before a transaction is completed. Note that, for transactions
containing multiple errors, this hook is called once for each of
the duplicate transactions that is used to report each of the
errors. This allows the context to be customised for each error.
@unflxw
Copy link
Contributor Author

unflxw commented Jan 9, 2025

@tombruijn I'm interested in any thoughts about the transaction after_create and before_complete hooks, especially if it seems like a bad idea to add these.

@unflxw unflxw force-pushed the ownership-integration branch from 64de16d to b822771 Compare January 9, 2025 16:12
Add an instrumentation for the `ownership` gem. When a transaction
is created, if an owner has been set in the gem, use the name of
the owner as the namespace for the transaction. Do the same when an
owner is set during an active transaction.

If more than one owner is set during a transaction, the namespace
will be that of the last owner that was set.
@unflxw unflxw force-pushed the ownership-integration branch from b822771 to ce2844a Compare January 9, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant