Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for decorating the client builder #290

Merged
merged 3 commits into from
Nov 7, 2019

Conversation

stayallive
Copy link
Collaborator

@stayallive stayallive commented Oct 24, 2019

This allows the user to add this snippet to for example their AppServiceProviders register method to set a custom serializer or do other tweaks to the ClientBuilderInterface; implementation or swap it out for their own builder interely.

In this example we increase maxDepth to 5 in for the default serializer.

use Sentry\Serializer\Serializer;
use Sentry\ClientBuilderInterface;

$this->app->extend(ClientBuilderInterface::class, function (ClientBuilderInterface $clientBuilder) {
    $clientBuilder->setSerializer(new Serializer($clientBuilder->getOptions(), 5));
    
    return $clientBuilder;
});

Resolves #289.

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I cannot honestly review this, it's out of what I know 😅

@stayallive stayallive merged commit 65cb908 into master Nov 7, 2019
@stayallive stayallive deleted the feature/allow-client-builder-modifications branch November 7, 2019 10:36
@smknstd
Copy link
Contributor

smknstd commented Nov 13, 2019

@stayallive as it has been released, did you plan to mention this in the docs ?

@stayallive
Copy link
Collaborator Author

@smknstd, yes. It’s taking a little time though. I hope for now the description of the PR will do until the docs can be updated, if you need any clarification let me know because that might benefit the documentation changes later on.

@smknstd
Copy link
Contributor

smknstd commented Nov 13, 2019

IMO Laravel section in the docs should mention context's additional data. Of course the job belongs more to laravel and PSR3, but I think it's an important feature and I'm pretty sure it could help to mention it explicitly. It could also mention more about it like:

  • the 4096 characters size limit
  • max depth set to 3 by default, and the fact that sentry hide all array data when the actual depth is too big.

It's really frustrating to discover those behaviors afterward...

Maybe a snippet like this could help:

Log::channel('sentry')->error('something',[
  "a_string" => "value",
  "an_array_with_limited_depth" => [
     "depth1" => [
        "depth2" => "value",
     ]
   ]
]);

And then

if you need to log an array with depth deeper than 3, ...

@mfn
Copy link

mfn commented Jan 18, 2021

I tried this and it "sort of" works, or at least, not in the way I expected it:

  • I had to use both ->setSerializer and ->setRepresentationSerializer
  • the UI change I got went from
    image
  • to
    image
    I thought I would also get back more collapsing functionality ;)

I wasn't sure if it's worth opening a dedicated issue, maybe that's simply the way it works but I found nothing in the docs regarding the setRepresentationSeralize at https://docs.sentry.io/platforms/php/guides/laravel/usage/#customization

thanks!

@stayallive
Copy link
Collaborator Author

@HazAT do you happen to know (or who to ask) if there is a server side limit to how many nested fields are shown / collapsable from the sentry.io interface?

Because it looks like there is more data in @mfn screenshots but de UI decided to ignore it possibly.

@Jean85
Copy link
Contributor

Jean85 commented Jan 18, 2021

@stayallive it seems more to me that he changed how the serializer works in the wrong way.

@mfn there's a specific $maxDepth constructor argument for the serializer that decides how much to go deep while serializing something like that, and the default value is 3. Upping that value changes the behaviour as desired.

@mfn
Copy link

mfn commented Jan 18, 2021

@stayallive @Jean85
Why did I not include the code 🤦 I was reporting this based on this code, apologies for not posting this immediately:

        $this->app->extend(ClientBuilderInterface::class, function (ClientBuilderInterface $clientBuilder): ClientBuilderInterface {
            $clientBuilder->setSerializer(new Serializer($clientBuilder->getOptions(), 5));
            $clientBuilder->setRepresentationSerializer(new RepresentationSerializer($clientBuilder->getOptions(), 5));

            return $clientBuilder;
        });

@mfn
Copy link

mfn commented Jan 27, 2021

sorry for the ping @stayallive @Jean85 but can you check my last comment? ☝️
Or should I create a new issue?

@stayallive
Copy link
Collaborator Author

@mfn, sorry I missed your response, it looks like you are doing it right but my assumption is that this is a UI "problem" and that the Sentry UI might not allow more nesting to show.

Could you share (the relevant section) of the event JSON to make sure it does come across correctly nested?

And yeah it's probably a good idea to move this to a new issue so we don't discuss this on a pretty unrelated PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Increase displayed depth of context's arrays
5 participants