Feat performance #145
Feat performance #145
Conversation
rommelfreddy
force-pushed
the
feat-performance
branch
2 times, most recently
from
b0050ce
to
eb756d0
Compare
rommelfreddy
force-pushed
the
feat-performance
branch
from
eb756d0
to
f8eac4e
Compare
rommelfreddy
force-pushed
the
feat-performance
branch
from
97573d4
to
2c696dd
Compare
rommelfreddy
force-pushed
the
feat-performance
branch
from
2c696dd
to
8995a2f
Compare
|
hey @indykoning could you give me a first response on this, please? I will resolve the conflicts if you are willing to proceed with the merge. |
|
He's currently with vacation, back next week |
| private ConfigInterface $config, | ||
| private array $excludePatterns = [] | ||
| ) { | ||
| $this->excludePatterns = array_merge([ |
There was a problem hiding this comment.
Could you add a comment here why these are excluded, so if we or others look back at this we can quickly see why
There was a problem hiding this comment.
i think the excluded events are self-explained.
these events happens very often on each request.
And because the count of spans in a transaction is limited, we have to exclude a few events, which also creates spans in a high amount.
| ]); | ||
|
|
||
| // Start the transaction | ||
| $transaction = startTransaction($context); |
There was a problem hiding this comment.
Just to make sure. Starting a transaction here won't count towards your Sentry usage and limits right?
Since we'll be executing it every request
There was a problem hiding this comment.
no. it only counts if the report is sent to the gateway (at the latest moment)
But i did not tested it explicitly, because we do not have any limits.
| } elseif ($state->getAreaCode() === 'graphql') { | ||
| $this->transaction->setOp('graphql'); | ||
| } else { | ||
| $this->transaction->setOp($state->getAreaCode()); | ||
| } |
There was a problem hiding this comment.
Was there anything special that needed to happen here? If not we can remove the elseif
| } elseif ($state->getAreaCode() === 'graphql') { | |
| $this->transaction->setOp('graphql'); | |
| } else { | |
| $this->transaction->setOp($state->getAreaCode()); | |
| } | |
| } else { | |
| $this->transaction->setOp($state->getAreaCode()); | |
| } |
| SentrySdk::getCurrentHub()->setSpan($transaction); | ||
| } | ||
|
|
||
| public function finishTransaction(ResponseInterface|int $statusCode): void |
There was a problem hiding this comment.
Should we maybe wrap this in a try/catch as well like captureException so issues when collecting performance metrics can't interfere with rendering the page to a customer
|
Is there any update on this PR? When can we expect this to be merged? |
@igorgaucho10 currently i do not have the time to change the PR. If you are using Versio 3.7.2, you can safefly use this patch: https://github.com/justbetter/magento2-sentry/compare/3.7.2...webidea24:magento2-sentry:master.diff I also use this in my projects. If you want, feel free to create a PR to my repository to resolve the comments of @indykoning |
related to #106
fixes #60
i've continued the implementation of the performance tracking and also added the profiling.
Thank you to @barryvdh for doing the base stuff