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

feat(experimental): adds transaction started and transaction closed hooks to the middleware. #987

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

Conversation

jcchavezs
Copy link
Member

This is specially useful when you want to use transaction data to populate things like logs or spans in a trace enriching the experience and connecting the dots with observability.

This is still ongoing. I am not 100% sure the TransactionClose method has any utility besides e.g. adding a span event for start and close or do something with MatchedRules maybe? (request for comments @anuraaga @jptosso @M4tteoP @MrWako)

…ooks to the middleware.

This is specially useful when you want to use transaction data to populate things like logs or spans in a trace enriching the experience and connecting the dots with observability.
Copy link
Contributor

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not 100% sure the TransactionClose method

I feel the opposite, I'm not sure what I would do in transaction open (I can't imagine opening a span since almost anything that would be a coraza transaction seems like it would be a span based on other instrumentation), but the close handler could possibly populate some paranoia score level or similar into a span attribute which seems potentially useful.

Since there's no linked issue I'm not sure if there's already a presented use case for it, I would probably hold off until seeing demand. If there is though, the change itself seems reasonable, think it can go in WAF itself instead of HTTP middleware-specific

// OnTransactionStarted is called when a new transaction is started. It is useful to
// complement observability signals like metrics, traces and logs by providing additional
// context about the transaction.
OnTransactionStarted func(tx plugintypes.TransactionState)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if it's fine for these not to return context, if not then started is probably even less useful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you thinking of injecting the tx into the context and passing down to the next middleware? The reason why I added this hooks to avoid the addition to yet another middleware to do stuff with the transaction.

@jcchavezs
Copy link
Member Author

Thanks @anuraaga for the feedback. Indeed there is no linked issue, I opened this PR to grab feedback but no intention to merge until someone request it.

@MrWako
Copy link

MrWako commented Feb 8, 2024

Hi @jcchavezs - I can see the use cases you had in mind but for the timing being I don't think I have a requirement for them. thanks.

@jcchavezs
Copy link
Member Author

Something to relate to is also the discussion on this PR #774

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.

3 participants