Skip to content

CaptureContext in captureMessage disallows the level to be set. #2819

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

Closed
5 of 9 tasks
stuckj opened this issue Aug 13, 2020 · 12 comments
Closed
5 of 9 tasks

CaptureContext in captureMessage disallows the level to be set. #2819

stuckj opened this issue Aug 13, 2020 · 12 comments

Comments

@stuckj
Copy link

stuckj commented Aug 13, 2020

Package + Version

  • @sentry/browser
  • @sentry/node
  • raven-js
  • raven-node (raven for node)
  • other:

Version:

5.21.0

Description

This issue here describes the bug I'm going to explain below.

The change in captureMessage in the node SDK introduced in #2627 changed captureMessage to be able to accept a context to apply to the message. This is handy. However, you can't specify both a context and a severity. I tried to do so by simply specifying the level in the context, but I'm seeing the error in the linked issue above that indicates that the value for level was discarded because it was invalid. I believe it was telling me it was blank (though that wasn't super clear in the error).

I traced through the code a bit and it appears this is because the generated JS parses out EITHER the level or the context and passes them on to your hub function. This is what I see in the generated JS (from your TS) off npm:

function captureMessage(message, captureContext) {
    var syntheticException;
    try {
        throw new Error(message);
    }
    catch (exception) {
        syntheticException = exception;
    }
    // This is necessary to provide explicit scopes upgrade, without changing the original
    // arrity of the `captureMessage(message, level)` method.
    var level = typeof captureContext === 'string' ? captureContext : undefined;
    var context = typeof captureContext !== 'string' ? { captureContext: captureContext } : undefined;
    return callOnHub('captureMessage', message, level, tslib_1.__assign({ originalException: message, syntheticException: syntheticException }, context));
}
exports.captureMessage = captureMessage;

When you pass a context to captureMessage, the callOnHub will get an undefined level and the context. Then in your hub handler (see generated JS below):

    Hub.prototype.captureMessage = function (message, level, hint) {
        var eventId = (this._lastEventId = utils_1.uuid4());
        var finalHint = hint;
        // If there's no explicit hint provided, mimick the same thing that would happen
        // in the minimal itself to create a consistent behavior.
        // We don't do this in the client, as it's the lowest level API, and doing this,
        // would prevent user from having full control over direct calls.
        if (!hint) {
            var syntheticException = void 0;
            try {
                throw new Error(message);
            }
            catch (exception) {
                syntheticException = exception;
            }
            finalHint = {
                originalException: message,
                syntheticException: syntheticException,
            };
        }
        this._invokeClient('captureMessage', message, level, tslib_1.__assign(tslib_1.__assign({}, finalHint), { event_id: eventId }));
        return eventId;
    };

It just passes the level on through to the client. The code's internal at that point, but I'm guessing you're just basically passing that on to your server with the separate level and context and your server is using the passed level and ignoring the level in the context.

I think you can fix this either in the server code (to use the context if the passed level is undefined) or in the node code by populating the level from the context if there is a level in the context (e.g, either in your hub method or the API method).

I can work around it for now by just wrapping the call with a withScope, it'd be nice to fix though since that was kinda the point of #2627. :)

@kamilogorek
Copy link
Contributor

From global singleton:

Sentry.captureMessage('wat', {
  tags: {
    some: 'value'
  },
  level: Sentry.Severity.Warning
});

From the hub:

const hub = Sentry.getCurrentHub();

hub.captureMessage('wat', Sentry.Severity.Warning, {
  captureContext: {
    tags: {
      wat: 1337
    },
  }
});

// or

hub.captureMessage('wat', null, {
  captureContext: {
    level: Sentry.Severity.Warning,
    tags: {
      wat: 1337
    },
  }
});

From the client: same as with hub, but it also accepts scope as the last argument, so can be done like so:

const client = Sentry.getCurrentHub().getClient();
const scope = new Sentry.Scope();
scope.setTag('wat', 1337);
client.captureMessage('wat', Sentry.Severity.Warning, {}, scope);

This API change was added after the major version release as the part of ongoing change, that's why it's not unified across all the layers. It's one of the things that we'll fix in the upcoming major v6 version - #2817

Cheers!

@stuckj
Copy link
Author

stuckj commented Aug 14, 2020

Thanks for showing the various ways to send the event. The first way is the way I was using and I think is where there is a bug. :)

Rather than using Sentry.Severity.Info, I was using just a string ('info') (good to know the enums exist though! But, otherwise I was doing exactly what you had here:

Sentry.captureMessage('wat', {
  tags: {
    some: 'value'
  },
  level: Sentry.Severity.Warning
});

And it resulted in the error I linked at the top of the report. I have it working now with withScope now though, but I think there might still be a bug there.

The majority of the error report described what I thought was causing it.

@kamilogorek
Copy link
Contributor

The issue that you linked (which cased a parsing error on the server-side) is using SDK version 5.15.5. Explicit scopes have been introduced in 5.16 😅

@stuckj
Copy link
Author

stuckj commented Aug 17, 2020

I don't track your change log closely enough to know the version the bug was introduced. I only pointed out where I think the commit was that introduced the bug. But, I can observe the bug on the latest version. Feel free to try for yourself or leave it as is if you wish. I have a workaround, but it looks like there's a bug in there as I described. :)

Have a good one,
-Johnny

@kamilogorek
Copy link
Contributor

What I meant is that the issue you linked used the SDK version where this API didn't even exist, so it's given that it'll produce an error.

I just tried the code from my initial answer using the lastest version:

Sentry.captureMessage('wat', {
  tags: {
    some: 'value'
  },
  level: Sentry.Severity.Warning
});

And it produces no error. Am I missing something?

@stuckj
Copy link
Author

stuckj commented Aug 17, 2020

🤦 The confusion here is my fault. I didn't realize I linked the top level issue rather than the specific event. Let's start over. :)

This is the specific event that exhibits the error. The error is not returned in code from the SDK. What I meant is that I see an error in the sentry console itself for the event. Here's a picture of exactly what I mean:
Screenshot from 2020-08-17 16-25-35 The level of the message also is defaulted to error instead of the level I was assigning in the scope (info).

I was doing this to capture that message:

Sentry.captureMessage(msg, {
  tags: { ...tags, logMsg: true },
  extra: { ...extra, msg },
  level
})

tags is extra tags to add to the message, level is a log level passed in, msg is the message to log, and extra is any other extra data to log with the message.

Using similar code as above for captureError works as expected so this seems specific to captureMessage.

Using this code for captureMessage instead works as expected where the level will be set to what is passed in (I was using info for the events associated with that issue):

Sentry.withScope(scope => {
 scope.setTags({ ...tags, logMsg: true })
  scope.setExtra({ ...extra, msg })
  // eslint-disable-next-line no-undef
  Sentry.captureMessage(msg, level)
})

All the analysis from my initial message was about why this might be (i.e., that captureMessage takes the scope OR the level and the helper function from there initializes the level from the second parameter or undefined if it's not a string.

The 5.15.5 report in the sentry UI is due to a pinned version from another dependency we have (gatsby-plugin-sentry). It was using @sentry/browser latest and we hadn't upgraded it so it was stuck at 5.15.5 in yarn.lock. We separately have @sentry/node 5.21.0 in package.json and it appeared to be what was used since I could use the newer interface. However, perhaps that's leading to a conflict. I'll resolve that and re-test to see if I still see the same behavior.

For reference, we're using React with gatsby as our stack.

@stuckj
Copy link
Author

stuckj commented Aug 17, 2020

Thanks for pointing out the 5.15.5 version. The issue was a library version conflict. 5.15.5 was being referenced through a dependency from gatsby-plugin-sentry, but we were explicitly including @sentry/node with version 5.21.0. It worked for captureError, but I'm guessing something was screwed up with both libraries being included in the bundle. I made sure everything's on 5.21.1 and it's working great now. Either that or it was fixed from 5.21.0 -> 5.21.1. :-P In any event, it looks like it's working now.

Thanks for your help.

@kamilogorek
Copy link
Contributor

Great to hear that! I was wondering if I'm missing something or I cannot understand a basic question anymore 😅
Cheers!

@stuckj
Copy link
Author

stuckj commented Aug 18, 2020

Unrelated to the initial bug, but that issue still shows 5.15.5 for new events even with our library updated. Perhaps the version is determined by the first event or is related to the merging of events into a related issue? E.g., this guy was logged today and definitely is from the 5.21.1 version of the SDK: https://sentry.io/organizations/goodpath/issues/1839387297/events/ec82147e1e92486697c21b9d43eab2c1/?project=1812305&statsPeriod=7d

@kamilogorek
Copy link
Contributor

kamilogorek commented Aug 19, 2020

@stuckj the event that you linked, points to deploy preview 1652... (don't want to use full URL publicly without your permission). When you open that page and verify the network tab, you can confirm that the SDK is using 5.15 in there, so it's coming from that older version to the server already:

image

Maybe some stale cache? on netlify?

@stuckj
Copy link
Author

stuckj commented Aug 19, 2020

Ah, it was an event from a deploy preview instead of production. I see the latest events in that issue now are properly showing 5.21.1. All looks good now. Thanks for your patience. :)

@kamilogorek
Copy link
Contributor

No worries, cheers! :)

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

No branches or pull requests

2 participants