Review and finalize version 1.0
mindplay-dk opened this issue · 34 comments
The 1.0.0 branch has been posted for review.
Please use this issue to post your comments or thumbs-up.
One point of early criticism from @thomasnordahl-dk is that this class is very large and ha a lot of responsibilities - this is actually "by design", since this client effectively has one API function and performs a single function.
However, I can already see a need for some minor kodus-specific (and kodus/user-session-specific) extensions - and it would be nice to be able to plug these in as simple extensions, composition over inheritance and all that.
We now have a simple extension interface and a some smaller components to address this:
https://github.com/kodus/sentry/tree/1.0.0/src/Extensions
The client constructor accepts an array of these, and runs them in order whenever an event gets captured. The Event model and it's components are mutable by design, so these extensions will simply take turns looking at the error/request instances and updating the Event model.
I have looked into the code and tests. I didn't know the postman-echo.com service, and php_uname method. I am not sure if I like the external dependency on a remote service to be online for tests to pass. I don't know if this is the reason for the "unknown" tag from Travis?
Should we mention something about the UserInfo model? Right now the only property to be set is the ip-address. If people want more information, they need to overwrite the applyRequestDetails in SentryClient, this may be a common use case?
In general, it looks very good, and I am looking forward to use it.
I am not sure if I like the external dependency on a remote service to be online for tests to pass
postman-echo.com exists solely for the purpose of testing clients :-)
Ideally, the test-suite should launch some kind of local server and post to that, but that's quite a bit of work to set up - the fetch function is trivial, and unlikely to change, so not really worth the time.
I don't know if this is the reason for the "unknown" tag from Travis?
The badge is for master, which doesn't exist yet - it'll go back to normal on merge.
Should we mention something about the UserInfo model? Right now the only property to be set is the ip-address. If people want more information, they need to overwrite the applyRequestDetails in SentryClient, this may be a common use case?
Hence the question about EventProcessor and allowing some kind of composable behavior.
We could add an example demonstrating how to populate UserInfo, but I'd like to decide first if that example should be an override of applyRequestDetails() or an implementation of some sort of interface.
Replace key by something that it obviously not a key - maybe add docblock with instructions on how to obtain a key and what you might need it for.
Please add a link with an explanation
I wonder why you have to silence php errors?
I suppose these are redefined to test under the same conditions each time?
Is there a test that fails when Windows 11 is released?
D fe koo!
I wonder why you have to silence php errors?
@jrmoller this suppresses an expected E_NOTICE when unpacking an array with fewer than the expected number of arguments - for example:
$bar = "baz"; // will be overwritten
@list($foo, $bar) = ["foo"]; // overwrites both $foo and $bar
var_dump($foo, $bar); // outputs "string", nullI suppose these are redefined to test under the same conditions each time?
Correct :-)
Is there a test that fails when Windows 11 is released?
Heh, no. The word on street is Windows 10 is the final generation of Windows - they'll continue with incremental updates from here on out, same as most other operating systems.
Either way, if there's a major new browser or relevant new OS, of course we'll have to add a line to this.
(Note that there are tests in place for unknown OS and a fallback for unknown browsers.)
@boan2010 @jrmoller would either of you care to comment on the extension question?
Just a note to @jrmoller, the original extension interface is dead (EventProcessor).
The client is updated, and takes a collection of extensions in the constructor.
The extensions can be found here: https://github.com/kodus/sentry/tree/client-extensions/src/Extensions
At my first glance, it looks like a better organized structure.
I've merged the refactorings for extensions.
Still working on the collection of breadcrumbs, for which I'll do a separate PR soon.
Just a note to @jrmoller, the original extension interface is dead (EventProcessor).
The client is updated, and takes a collection of extensions in the constructor.
The extensions can be found here: https://github.com/kodus/sentry/tree/client-extensions/src/Extensions
I tried to follow the links, but got 404s in both cases, but thanks @boan2010 for the information that the EventProcessor is dead :-)
I spoke with @mindplay-dk about the refactoring of the client, where parts of the captureException method are delegated to extensions. I think it is nice that the details included in the logging can be customized per project by defining and including extensions. It probably requires some investigation of the interplay between the event model and the client to understand how to write the apply function of a new extension though.
Maybe just add to the documentation, that an extension can be written by defining a context (implementing the context interface) and how you must use the addTag and addContext methods on Event to include something in the logging.
After talking to @mindplay-dk I understand that you have to look closer at the sentry documentation - e.g. https://docs.sentry.io/clientdev/unified-api/, if you want to develop an extension.
@jrmoller I've added the missing Event properties from the Sentry schema - these (empty) properties provides space for custom extensions to populate with more information, if needed.
I've also ported the BreadcrumbLogger to a proper extension, and made it responsible for collecting PSR-5 log-entries as "breadcrumbs" in Sentry.
I think this should be the last round of changes for now.
Fine, but in line 37 of the Breadcrumblogger and here:
I've also ported the
BreadcrumbLoggerto a proper extension, and made it responsible for collecting PSR-5 log-entries as "breadcrumbs" in Sentry.
You say PSR-5 where I think it should be PSR-3?
@jrmoller thanks, I get these numbers mixed up all the time
Looking very good!
I do have a tiny reservation about the usage of mock classes in tests. In principle, by testing MockSentryClient, we can only claim to have tested this class. Of course this is not true in practice, but I do have some reservations to this technique.
I recognize testing stuff like timestamps can be difficult without establishing a whole factory concept or something like this. But perhaps to minimize the risk of bypassing errors unnoticed, we could require the Mocks to call parent methods if possible.
E.g. the parent createEventID() method could be called, and a typecheck/validation could be performed before returning a mock value from the extending method.
I'm not sure, what do you think?
In general, I'm all for a release tag. Looks good!
P.S. I'm not sure we should add the opinionated parts of the README.md to the public kodus repositories. Maybe we should stick to the cold ("what is it? / "how do I use it?") facts?
I recognize testing stuff like timestamps can be difficult without establishing a whole factory concept or something like this.
I see your point, and I don't disagree with the principle of it - but when it comes to testing, the investment has to be worth while.
Compared with injecting mock abstractions, in terms of testing the actual implementation of something, there's literally no difference between that and overriding a protected method - you're still not testing the implementation that end-users will actually use, so.
In this particular case, I find that requiring the consumer to create and inject a clock abstraction is extra complexity that provides no value to the consumer - he's never going to need to inject anything other than a real clock for any reason. So the abstraction doesn't provide any value for the consumer, it just gets in the way.
Tests shouldn't drive architecture, in my opinion - "testable" is not a quality that should lead to unnecessary extra architecture and complexity.
That is, I like writing "testable" code, but only when "testable" coincides with "useful" or "meaningful".
To your other point, yes, some things aren't tested. I don't aim for 100% test-coverage - in fact, I don't even measure test-coverage. I try to test anything that is a potential liability. But there's literally no way you can mess up something like return time() - for trivial things like this, I find a test has no value.
Something like the UUID creation is arguably more valuable to test - however, it's using a somewhat unorthodox format (without hyphens) that was mandated by Sentry for some reason. I couldn't write a (unit/integration) test that actually tests whether Sentry accepts the value, so I didn't write one - I tested it against the actual Sentry service, verified that it worked, and then didn't touch that code again.
An E2E test would definitely have value, but I'm not sure it's "nice" to the Sentry folks (or even allowed?) to publish a private key with the test-suite. (?)
(Disclaimer: We are getting into test philosophy, and maybe moving out of the scope release tagging - I'm not trying to argue for a refactoring of the tests here, just wanted to explore your comments on tests).
In my view, by depending on something like a Clock class for timestamps, in essence you remove the timestamp responsibility from the unit under test. We can then test that whatever Clock is given to the unit under test is applied correctly, and we can then make a separate unit test that tests the Clock class independently.
This way we can test all aspects of the different units independently and fully.
I agree that tests shouldn't "drive" the architecture, but I definitely use it as a quality benchmark. If I can't test an aspect of the code design, I cannot truly claim it is working. In my mind, if I've made a design that is difficult to test, I have more work to do on that design.
Sometimes, we throw in the towel, and say "It would require a whole framework / factory architecture to test that the timestamps are precise". But this will always be a trade off.
So I check code coverage, and I aim for 100%. I never reach 100%, but it almost always shows me some cases I forgot to test. "Shoot for the moon. Even if you miss, you'll land among the stars."
Why redefine captureEvent in MockSentryClient? (you're making it public instead of protected, but I don't think the method is called from any test)
I wonder whether the call to captureException should be wrapped in a try catch since fetch potentially throws a runtimeexception? (At least for notices and warnings - otherwise our applications might crash due to the Sentry dashboard being down?)
@jrmoller junk from an earlier iteration - removed
I wonder whether the call to captureException should be wrapped in a try catch since fetch potentially throws a runtimeexception?
This is a good point, what do we do if the back-end is down? Quietly ignore it?
I wonder whether the call to captureException should be wrapped in a try catch since fetch potentially throws a runtimeexception?
Nice catch @jrmoller :-)
@boan2010 @jrmoller @thomasnordahl-dk any other comments, or can we ship this?
No new comments from me...
Ship it!