Chore/implement global max trace items #89
Conversation
|
Looking good, thanks for the fix! I have nothing really to add, looking forward to the PR 👍 |
|
I think this is ready. I'd like to spend time on the event collectors, but that's better done in separate PRs. I see there is a new failing check for styleci, but I get a 404 when I try to view the details for it. What needs to be done for that to pass? |
countless-integers
changed the title
|
My spider sense tells me that if you push anything to this branch this check won't be triggered any more. I think @arkaitzgarro disabled it for now. |
Yes, I disabled it. I wanted to know if the Laravel preset was linting variable cases, but it didn't. Looks like an interesting service though, will take a look in the future. |
dstepe
force-pushed
the
chore/implment-global-max-trace-items
branch
from
1c62d5d
to
4e2befe
Compare
|
That's a good opportunity fix a typo in the README and clean up the commits. All is good now. |
|
@arkaitzgarro This should be ready now if it looks good to you. |
dstepe
force-pushed
the
chore/implment-global-max-trace-items
branch
from
4e2befe
to
c69ea15
Compare
|
I rebased on to master and fixed a unit test impacted by my changes. |
dstepe
force-pushed
the
chore/implment-global-max-trace-items
branch
from
7d789e8
to
07d01c8
Compare
|
How about I rebase this branch onto master again and see how it looks? I think that might be better than merging master into the branch. I have a little bit of time now to look at it. |
This led to some work on testing the EventCollector abstract class and adding an EventClock to enable controlling time.
This required adding a private EventCollector::pushMeasure() method for addMeasure and stopMeasure to use when the limit has not been exceeded.
dstepe
force-pushed
the
chore/implment-global-max-trace-items
branch
from
c3bd82f
to
8397db2
Compare
|
My apologies. I saw the some of the other branches being merged and began making the fixes for the new Collector tests on this branch. That caused the conflicts you experienced. I rebased on to the latest master, cleaned up the tests and force pushed. All the Collector tests are working now. |
Closes #85
This took a little more work than I expected, but I think it covers the expected use cases. I'm still looking at EventCollector tests and want to make note about some behaviors in the docs. I wanted to get this out as a draft, though.
I replaced the
CustomCollectorTestwithEventDataCollectorTest. The original test did not seem to be actually testing a "custom" collector, just using it as a way to test the abstractEventDataCollector. I believe this approach is more explicit and obvious.The use of the
microtime()function makes testing difficult, so I added anEventClockclass which is injected where needed and will provide the time.The one caveat to how the item limit works relates to when events are added. If a collector were to collect events during the request, but only call
addMeasure()at the end of the request, the events may not be collected even if they occurred earlier in the transaction. (This is how the original philkra Laravel package handled DB queries for example.) I don't know that anyone will ever run into this, but I want to figure out how to explain it in the docs just in case.