Add support for contexts #839

williamdes · 2019-06-25T15:52:42Z

https://github.com/getsentry/sentry-javascript/blob/faf038bef8d3812982f258e8630d7d3521a9ff02/packages/integrations/src/vue.ts#L93

https://github.com/getsentry/sentry-javascript/blob/c3b794026591ad40325f38250b852070f21c2982/packages/browser/test/index.test.ts#L45

setContext is missing
Contexts are very useful, the javascript sdk implements it correctly.

williamdes · 2019-06-25T15:53:28Z

Maybe my works is not perfect, I can improve it though.

williamdes · 2019-06-25T15:55:55Z

"contexts":{"akey":{"0":"avalue !!","type":"akey"},"keyscope":{"0":"efniurnuenrgergn","type":"keyscope"},"os":{"type":"os","name":"Linux"},"akey2":{"3":"ok","type":"akey2","4":{"rf0":"ok","key6":{"a":"b"},"key5":["ok"],"key4":"value"}}

The search builder in Sentry GUI

Jean85

I'm not sure about the usefulness of this feature; I would require @HazAT to chime in to understand how this feature should works in regard to Sentry server side.

src/State/Scope.php

williamdes · 2019-07-01T07:28:11Z

@HazAT @Jean85 Can you please review ?

src/State/Scope.php

src/Event.php

HazAT · 2019-07-01T08:07:27Z

Thanks for the PR, good job, we definitely want to have setContext. I left a few notes.

williamdes · 2019-07-01T08:57:20Z

@HazAT I did the changes you suggested, please review one last time :)

HazAT

LGTM, thanks again 👍
Maybe another review of @stayallive @Jean85 @ste93cry (sorry for the multi ping, one review should be enough)

@williamdes Please one last thing, add a changelog entry

williamdes · 2019-07-01T12:14:50Z

@HazAT While reviewing I found out that I had forgot to add some lines (b60017b)

Changelog added by ac56ad2

src/Context/Context.php

src/State/Scope.php

tests/State/ScopeTest.php

src/State/Scope.php

ste93cry · 2019-07-01T22:03:45Z

src/State/Scope.php

+    public function setContext(string $name, array $value): self
+    {
+        if (!isset($this->contexts[$name])) {
+            $this->contexts[$name] = new \stdClass();


I don't really like to have to use stdClass, what's the point of doing it instead of using $value directly?

Sentry only accepts objects for contexts, any other value will make an error..

This is reasonable only if the user is using numeric-only keys, so json_encode will mistake it for an array.

@wedamija you're right, but there is only one case that cames to my mind that could cause PHP to JSON-encode in the wrong format the array: when it represents a pure list (so 0-based index). In all other cases it should work, and I expect people to use the context more as a map than a list. Of course this cannot be strictly enforced, we could add a check like in the TagsContext that the key is a string until PHP will have true generics that will allow typehinting the arrays

@ste93cry What should I do ?

we could add a check like in the TagsContext that the key is a string

I'd be in favor of doing so as well.

This won't work as PHP will convert a string containing a number into a number if it's used as key of an array 💩 Imho let's not complicate the code to try handle such cases, make use of a simple array instead of stdClass and it will work fine as long as someone does not really pass a list instead of a map

@ste93cry do I need to make a change here ?

as I said "Sentry only accepts objects for contexts, any other value will make an error."

do I need to make a change here ?

Yes please, let's start with array and let's see what happens and how the situation evolves in the future. Also please update the code to use a plain array for the contexts variable and add the setContexts method to the Event class.

"Sentry only accepts objects for contexts, any other value will make an error."

You're right, however it's unlikely that someone will put a list as value of a context and it's unlikely that someone will use string keys containing numbers knowing that they will be casted

Okay, made the required changes (rebase, amend fix)
See 706fd14

williamdes · 2019-07-01T22:35:14Z

@ste93cry I made the changes you requested :)

ste93cry

ping @HazAT one thing that doesn't convince me of this feature is that it will allow people to just override whatever is in the contexts key, even the "standard" ones like server OS or request that cames from the interfaces. Right now the SDK interfaces are well defined and invalid data cannot be set while with this change the whole validation will be unuseful. Also I find really confusing that there are multiple ways to configure the same thing (the particular contexts and the contexts as a whole thing)

src/State/Scope.php

src/Context/Context.php

CHANGELOG.md

src/Event.php

williamdes · 2019-07-11T13:27:15Z

@ste93cry I made more changes as you requested.

one thing that doesn't convince me of this feature is that it will allow people to just override whatever is in the contexts key, even the "standard" ones like server OS or request that cames from the interfaces.

Seems okay for me, you control the data you send to Sentry, no ?

Also I find really confusing that there are multiple ways to configure the same thing (the particular contexts and the contexts as a whole thing)

Same thing ?
They are not in the same place in the Sentry GUI and in the request
Did I misunderstand ?

ste93cry · 2019-07-11T14:25:04Z

They are not in the same place in the Sentry GUI and in the request

What I meant is that some data like the information on the OS of the server or the HTTP request is stored under the $contexts key, which with this feature can be completely overridden by the user without any sort of validation. This means that before the data sent to Sentry was always filled correctly and strictly validated (if the server did not expect a string you could not send a string) while now we are going to lose all of these stuffs because we are just going to blindly replace all data with something else given by the user, and since some of the default contexts are named with common words (server, request, etc) it will be really easy to lose useful data by mistake

williamdes · 2019-07-11T14:40:10Z

default contexts are named with common words (server, request, etc) it will be really easy to lose useful data by mistake

I agree

ste93cry · 2019-07-11T15:22:47Z

To clarify some more things, when I talked about strict validation I meant that the data of the contexts key of the event's payload are handled by specialized classes (ServerOsContext, RuntimeContext, etc) that validate the data (both types and allowed values). By just replacing its value with random data from the user we're losing such feature. Also, supposing you're not passing all required attributes for those special default contexts or you are passing more attributes than what the server expects we are gonna break the logging of events without the user be ever notified of this because we don't log the errors during the sending. In this regards, the classes were introduced to have both a nice API to manage each context and to prevent such issues by validating and restricting what the user could do on them. This is why I undoubtedly see the value of having such feature but I also see that it's going to introduce a lot of pitfalls that won't be easily solved once we merge it as is because we won't be able to rollback without breaking the behavior, be it user or technical. If there was some restriction, for example in disallowing/ignoring the override of the default contexts, I would be totally fine. Of course, if everyone agree anyway with this PR I'm ok to merge it

williamdes · 2019-07-11T15:29:32Z

If there was some restriction, for example in disallowing/ignoring the override of the default contexts, I would be totally fine. Of course, if everyone agree anyway with this PR I'm ok to merge it

Okay, I someone else requests disallowing the override, I will invert the array merge (https://github.com/getsentry/sentry-php/pull/839/files#diff-a8ad00caade65c3a6d9d157e7e4b8708R656).

williamdes · 2019-08-07T12:16:31Z

rebased.

williamdes · 2019-08-30T21:50:53Z

rebased another time..

@HazAT @ste93cry @Jean85 Can you merge this feature or tell me what to change please ?

2 months later ..

ste93cry · 2019-08-30T22:20:27Z

This feature will be scheduled for 2.3 since there are some more general changes regarding how we manage contexts that may affect this feature and how it’s coded. I’m sorry if this is taking more time than you expected, in the meanwhile you can use the beforeSend option to edit the raw event data before it’s sent

williamdes · 2019-10-29T22:39:40Z

I have no idea why psalm complains about, I did not change this file..

ERROR: InvalidOperand - src/Stacktrace.php:314:36 - Cannot perform a numeric operation with a non-numeric type array-key
                $result['param' . ($index + 1)] = $this->serializeArgument($argument);

williamdes · 2019-10-29T23:02:10Z

@ste93cry I checked and the error shows on develop branch so I will not fix it.

ste93cry · 2019-10-30T08:18:20Z

src/State/Scope.php

+     * Sets context data with the given name.
+     *
+     * @param string               $name  The name that uniquely identifies the context
+     * @param array<string, mixed> $value The value


Shouldn't this be array<string, array<string, mixed>>?

I will try another time but I did have to revert this because psalm says it looks like an object

ste93cry · 2019-10-30T08:19:49Z

src/State/Scope.php

+    /**
+     * @var Context The list of contexts associated to this scope
+     */
+    private $contexts;


For simplicity let's use an array<string, array<string, mixed>> too here. I don't think that there is any good reason to use the Context object which we may remove in 3.0 given that `contexts is able to override any data of the interfaces, thus effectively bypassing the validation provided by these objects

ste93cry · 2019-10-30T08:21:16Z

src/Event.php

+     * Sets context data with the given name.
+     *
+     * @param string               $name  The name that uniquely identifies the context
+     * @param array<string, mixed> $value The value


The typehint should be array<string, array<string, mixed>>

ste93cry · 2019-10-30T08:22:45Z

src/Event.php

+    /**
+     * @var Context An arbitrary mapping of additional contexts associated to this event
+     */
+    private $contexts;


Please switch to array<string, array<string, mixed>>, see similar comment below for the explanation of why I'm requesting this change

ste93cry · 2019-10-30T08:23:46Z

tests/EventTest.php

@@ -274,6 +362,25 @@ public function testGetExtraContext(): void
        $this->assertInstanceOf(Context::class, $event->getExtraContext());
    }

+    public function testGetContexts(): void


I don't think that this test is useful because using typehints we are guaranteed that the returned value is of the expected type

ste93cry · 2019-10-30T08:48:54Z

tests/EventTest.php

@@ -119,6 +119,94 @@ public function testToArray(): void
        $this->assertEquals($expected, $event->toArray());
    }

+    public function testToArrayMergeUserContexts(): void


What about merging this test and the one below using a data provider?

Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr>

Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr> Add unit tests for Contexts Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr> Fix CR issues

williamdes · 2019-12-22T13:08:08Z

@ste93cry did my work get merged?

ste93cry · 2019-12-22T13:12:13Z

Not yet, I wanted to rebase and change some things before requesting a final review and merging but somehow I messed your branch or whatever it happened (I honestly don't know) and GitHub closed the PR because it though there were no more commits... I will open a new PR as followup of this one with all your changes plus mines merged together. I feel very sorry for how I personally handled your contribution, how long it took to get my attention and how long it's taking to get merged

williamdes · 2019-12-22T13:23:47Z

Thank you for your nice reply, keep me updated on the progress of this PR

I feel very sorry for how I personally handled your contribution, how long it took to get my attention and how long it's taking to get merged

I totally understand, I have the same challenges on the phpMyAdmin project with all the contributions and issues

Jean85 reviewed Jun 25, 2019

View reviewed changes

src/State/Scope.php Outdated Show resolved Hide resolved

ste93cry added Status: Needs Information Type: Feature labels Jul 1, 2019

HazAT requested changes Jul 1, 2019

View reviewed changes

src/State/Scope.php Outdated Show resolved Hide resolved

src/State/Scope.php Outdated Show resolved Hide resolved

src/Event.php Outdated Show resolved Hide resolved

HazAT approved these changes Jul 1, 2019

View reviewed changes

Jean85 approved these changes Jul 1, 2019

View reviewed changes

ste93cry changed the base branch from master to develop July 1, 2019 16:59

ste93cry requested changes Jul 1, 2019

View reviewed changes

ste93cry added this to the 2.2 milestone Jul 2, 2019

ste93cry added Status: Confirmed and removed Status: Needs Information labels Jul 2, 2019

ste93cry requested changes Jul 11, 2019

View reviewed changes

src/State/Scope.php Outdated Show resolved Hide resolved

src/Context/Context.php Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

src/Event.php Outdated Show resolved Hide resolved

src/Event.php Outdated Show resolved Hide resolved

ste93cry modified the milestones: 2.2, 2.3 Aug 30, 2019

williamdes requested a review from ste93cry October 29, 2019 23:02

ste93cry requested changes Oct 30, 2019

View reviewed changes

ste93cry closed this Dec 22, 2019

ste93cry pushed a commit to ste93cry/sentry-php that referenced this pull request Dec 22, 2019

Add CHANGELOG entry for getsentry#839

24d3c28

Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr>

ste93cry pushed a commit to ste93cry/sentry-php that referenced this pull request Dec 22, 2019

Add support for contexts in the Scope

9380911

Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr>

ste93cry pushed a commit to ste93cry/sentry-php that referenced this pull request Dec 22, 2019

Add unit tests for Contexts

fbd317e

Pull-request: getsentry#839 Signed-off-by: William Desportes <williamdes@wdes.fr>

ste93cry mentioned this pull request Dec 22, 2019

Support setting arbitrary contexts in the scope and event #941

Merged

Add support for contexts #839

Add support for contexts #839

Conversation

williamdes commented Jun 25, 2019

williamdes commented Jun 25, 2019

williamdes commented Jun 25, 2019 • edited

The search builder in Sentry GUI

Jean85 left a comment

Choose a reason for hiding this comment

williamdes commented Jul 1, 2019

HazAT commented Jul 1, 2019

williamdes commented Jul 1, 2019

HazAT left a comment

Choose a reason for hiding this comment

williamdes commented Jul 1, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamdes Oct 29, 2019 • edited

Choose a reason for hiding this comment

williamdes commented Jul 1, 2019

ste93cry left a comment

Choose a reason for hiding this comment

williamdes commented Jul 11, 2019

ste93cry commented Jul 11, 2019 • edited

williamdes commented Jul 11, 2019

ste93cry commented Jul 11, 2019

williamdes commented Jul 11, 2019

williamdes commented Aug 7, 2019

williamdes commented Aug 30, 2019

ste93cry commented Aug 30, 2019

williamdes commented Oct 29, 2019

williamdes commented Oct 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamdes commented Dec 22, 2019

ste93cry commented Dec 22, 2019

williamdes commented Dec 22, 2019

williamdes commented Jun 25, 2019 •

edited

williamdes Oct 29, 2019 •

edited

ste93cry commented Jul 11, 2019 •

edited