[2.0] Implement the Scope and Hub of the unified API

[ste93cry]

Q	A
Branch?	2.0
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
License	MIT

This PR aims at providing the Scope and Hub features of the unified SDK API. It's based on the work of @HazAT and improves it by adding all the needed docblocks and refactoring the code while improving at the same time the unit tests. There are a few inconsistencies of the API which I would like to discuss before merging:

• the capture* methods of the Scope do not follow the same pattern for the function definition: some let you set the severity level of the event, others don't. Also there is no way to pass a customized payload to the client (see the Client::capture* methods to understand what I'm referring to)
• the Scope class does not allows to set data on all contexts: runtime and server OS contexts are missing. Also the API of the scope doesn't expose the underlying context object which means that the methods developed to easy the management of the context data are not available to the end user.
• the Hub::applyToEvent method behaves differently from data to data being merged into the event. As an example, the breadcrumbs are not merged with the existing ones but the severity level always override the one set in the event. Why?

[HazAT]

When talking about capture* on the Scope you actually meant Hub, right?
So I know this is one of the bigger changes of the API. Before our capture* methods took a payload which can be anything the event takes?
While this maybe worked in PHP in other SDKs you were not able to pass anything here the event takes, only for example tags extra and so on, but not an exception with for example.

That's why we went with the approach to remove this "magic" payload parameter and introduced Hub::withScope and EventProcessors as a workaround for this.

---

If someone wants to change server or os context of an event, they should write their own integration or add an EventProcessor. We do not want to expose all possible getter/setter that is on the event. Instead, if someone wants to fundamentally modify the event they should, as said before an EventProcessor, an Integration or beforeSend callback.

---

Scope::applyToEvent wasn't just fully implemented by me, ultimately the Scope should be the single source of truth when it comes to the "State". Which means the current Scope holds all breadcrumbs, context data (most used) and EventProcessors. Before an event will be sent off to Sentry if should go through Scope::applyToEvent where we merge the "State" of the Scope onto the event.

[ste93cry]

When talking about capture* on the Scope you actually meant Hub, right?
So I know this is one of the bigger changes of the API. Before our capture* methods took a payload which can be anything the event takes?
While this maybe worked in PHP in other SDKs you were not able to pass anything here the event takes, only for example tags extra and so on, but not an exception with for example.

That's why we went with the approach to remove this "magic" payload parameter and introduced Hub::withScope and EventProcessors as a workaround for this.

Yes, I indeed meant Hub. It's fine if you can't use anymore the payload, but the methods are inconsistent anyway because for example the captureMessage accepts a level argument but the captureException not. I think we should normalize them (and in case normalize the API across all SDKs). For the same reason you should use Hub::withScope or event processors to customize the data of the event you should in case use them to customize the level. Does it makes sense?

If someone wants to change server or os context of an event, they should write their own integration or add an EventProcessor. We do not want to expose all possible getter/setter that is on the event. Instead, if someone wants to fundamentally modify the event they should, as said before an EventProcessor, an Integration or beforeSend callback.

Even though I understand that exposing all the getters/setter of the event on the Scope object is out of discussion, on which base we decide that setTag and getTags are fine to be on the scope but not the getters/setters for the other contexts (this is just an example, I could have written the same thing for any property of the event)? Probably the unified API should also unify the way you interact with the event and should be consistent without putting half of the setters/getters on the scope and half on the event

[Jean85]

This is a grea PR! Great work with the tests too!

I've commented a few things here and there, but nothing major...

[HazAT]

@ste93cry The basic idea is that the use case for captureMessage is for "logging".
For logging, it makes sense to define the level easily since it would be really annoying to do

Sentry.withScope(function($scope) {
  $scope->setLevel(Severity::debug());
  Sentry.captureMessage("test");
});

vs.

Sentry.captureMessage("test", Severity::debug());

In general, we want that the simplest use case of the SDK doesn't need any deep knowledge about Sentry at all. That's why we removed all the payload and second "magic" args from all other capture* methods.

There is setLevel on the Scope if you ever want to change the Level when capturing an Exception with captureException.

---

The idea of the unified API is to abstract away the complexity of the Client and the Event behind the Hub / Scope.
We still want users to be able to just create a new Client, instead of calling init, but then they are responsible for managing the State by themselves. If someone wants to fundamentally change the payload they have to go through an EventProcessor.

[ste93cry]

Imho soon or later someone will start asking why on the scope there are some methods and not others... Also the API doesn't provide any way except having to use an event processor to remove a tag or any data from the contexts exposed by the scope. It seems to me that the API aims to be simple and this is good, but half of the things can be set in some way and the other half can't. As a user my first question would be on what basis those methods were chosen and not others.

[HazAT]

@ste93cry So we decided on the public API what we think would be the most common use case for >90% of people using Sentry.
Most people install it, call init and that's it.
Maybe they set some Tags to filter or add extra data to have more context.

Just to elaborate on the advantages of small public API again:
We can easily change the internals of events and how we send them.

While we still wanted to maintain great flexibility if your usage of our SDK isn't the norm, it's fine, we want users to have ways around our limited public API but we will not deliver guarantees that the internal will work like this in a future version of our SDK.

The Public API we deliver is a contract we tend to keep, but if we someday decide for example to completely drop the OS Context (not that we actually want to do this) from the event and people where using setOSContext across all SDKs, we have to update all SDKs to be on par with our decision.

Ultimately it's about the smaller the public documented API the less maintenance we have to do.
We can always add more things later but then we do that in all SDKs.

[Jean85]

@HazAT we should probably state this (maybe concisely) on the README to clarify what's covered and considered as public API, in relation to BC and semantic versioning then.

[HazAT]

Great work so far, there are a few minor things but we will change them in the upcoming PRs. 🥇

[Jean85]

LGTM! 👍
I have added a few questions, but non are blockers for the merge.

[Jean85]

Only one minor PHPDoc fix, LGTM 👍

[HazAT]

Solid PR, there are some minor things we iron out in the PRs to come, no blocker.