feat(utils): Add `parameterize` function #9145

AleshaOleg · 2023-09-29T18:09:48Z

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

If you've added code that should be tested, please add tests.
Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

This PR solves issue #6725

Lms24

Hi @AleshaOleg thanks for opening this PR!

What you have until now looks promising but it seems, we're still missing a crucial part here to resolve #6725: We still need to process the __sentry_template_... fields in our browser and server SDKs. Happy to leave the details up to you but I think these functions need adjustments:

https://github.com/getsentry/sentry-javascript/blob/c07ab359c8db278dbb0ac08a6903747bf9f27eb9/packages/utils/src/eventbuilder.ts#L116-L150

sentry-javascript/packages/browser/src/eventbuilder.ts

Lines 263 to 286 in c07ab35

    
           /** 
        
            * @hidden 
        
            */ 
        
           export function eventFromString( 
        
             stackParser: StackParser, 
        
             input: string, 
        
             syntheticException?: Error, 
        
             attachStacktrace?: boolean, 
        
           ): Event { 
        
             const event: Event = { 
        
               message: input, 
        
             }; 
        
             if (attachStacktrace && syntheticException) { 
        
               const frames = parseStackFrames(stackParser, syntheticException); 
        
               if (frames.length) { 
        
                 event.exception = { 
        
                   values: [{ value: input, stacktrace: { frames } }], 
        
                 }; 
        
               } 
        
             } 
        
             return event; 
        
           }

Ultimately, we want to construct an Event where we populate the Message interface. @AbhiPrasad Not sure though what the difference between LogEntry and Message is though, do you know?

Feel free to tackle this in a follow-up PR. To test the entire functionality we should best add browser and node integration tests.

I think we also want to re-export parameterize from the SDK packages so that users can use them without explicitly importing from @sentry/utils.

AleshaOleg · 2023-10-02T21:07:25Z

@Lms24 Thanks for clarifying a bit! But still, there are some moments, which still remain not clear to me. As I got, I should add logentry along with exception for the event object in Browser API you mentioned. Am I right? For example here in Browser API:

sentry-javascript/packages/browser/src/eventbuilder.ts

Lines 279 to 281 in c07ab35

    
           event.exception = { 
        
             values: [{ value: input, stacktrace: { frames } }], 
        
           };

I should do something like this:

const formatted = parametrize`${input}`;
event = {
    ...event,
    "exception": {
        values: [{ value: input, stacktrace: { frames } }],
    },
    "logentry": {
        "message": formatted.__sentry_template_string__,
        "params": formatted.__sentry_template_values__,
    }
}

I got this idea from the first comment in the initial issue (#6725) which refers to this Python API code:

https://github.com/getsentry/sentry-python/blob/dd8bfe37d2ab369eaa481a93484d4140fd964842/sentry_sdk/integrations/logging.py#L253-L256

Sorry again, I'm not that familiar with Sentry API.

AleshaOleg · 2023-10-05T18:41:42Z

@Lms24 any update would be appreciated :)

Lms24 · 2023-10-06T15:03:42Z

@AleshaOleg I actually need to check back with some team members first, too. The whole LogEntry vs Message interface is a bit confusing. I'll get back to you on Monday.

Lms24

Hey @AleshaOleg I checked back with the team and now we know how to proceed. What's interesting to know is that event.message and event.logEntry alias each other in the Sentry backend so it technically doesn't matter which we populate with the {message, params} object. If both properties are specified, logentry will be preferred and message discarded.

However, to avoid breaking the Event.message type, we're going to populate event.logentry. This also means, that the Event type needs to be adjusted. You can modify the Event interface to specify that it either has a message?: string or a logentry?: {message?: string; params?: string[]} property. So basically make a mutually exclusive type definition. Also, both properties remain optional.

Also, for now, we make this parameterize function purely user-facing. So let's not call it within eventFromString (I'd argue this is already too late anyway, given that the input is just a string, no?).

What this means for this PR:

whenever the SDKs create an event from a string (e.g. via captureMessage), we check if the input string has the __sentry_template_... properties.
If they are present, we populate event.logentry
If not, we populate event.message with just the plain string
We adjust the Event type to either accept logentry, message or none of the two properties.

Does this make sense to you?

AleshaOleg · 2023-10-10T12:36:24Z

@Lms24 yes, I got everything. Now everything is clear to me, and I know the use-cases. But still not sure about this point:

If not, we populate event.string

Did you mean event.message there? If not, what is event.string then?

Lms24 · 2023-10-10T14:51:47Z

Did you mean event.message there? If not, what is event.string then?

Yes, good catch, thanks! I just misstyped. It's event.message. I edited my reply.

… feature/added-parametrize-function

AleshaOleg · 2023-10-11T20:15:00Z

@Lms24 updated PR, does it make sense what I did? If yes, I'll proceed with a couple of tests for the eventFromMessage functions.

Lms24

Yes, this makes sense to me! In addition to adding tests to the eventFromMessage functions, please also add browser and node integration tests. We already have integration tests for captureMessage, so adding a new one shouldn't be too hard:

We also need to adjust the API signature of eventFromMessage in the client interface:

sentry-javascript/packages/types/src/client.ts

Lines 158 to 165 in 0ffe696

    
           /** Creates an {@link Event} from primitive inputs to `captureMessage`. */ 
        
           eventFromMessage( 
        
             message: string, 
        
             // eslint-disable-next-line deprecation/deprecation 
        
             level?: Severity | SeverityLevel, 
        
             hint?: EventHint, 
        
           ): PromiseLike<Event>;

Lms24 · 2023-10-13T08:43:07Z

packages/browser/src/eventbuilder.ts

@@ -265,13 +265,11 @@ export function eventFromUnknownInput(
 */
 export function eventFromString(
  stackParser: StackParser,
-  input: string,
+  input: string & { __sentry_template_string__?: string; __sentry_template_values__?: string[] },


Can we extract ParemeterizedString from this PR as a type export it from @sentry/types? Then we can reuse it here.

Tried to replace it everywhere where I think it makes sense. Thanks for the tip!

…e to @sentry/types

AleshaOleg · 2023-10-13T23:21:46Z

@Lms24 I added integration tests, and now I understand better why there are still no eventFromMessage tests, as it's tested in the scope of captureMessage testing. Maybe I'll add a couple more tests, but for now, it's more or less finished PR and I want to ask you if could take a look as I made many changes.

Here is one tricky thing I did here - https://github.com/getsentry/sentry-javascript/pull/9145/files#diff-9b99586572e171cbe9f43e400ce8c631dfc032990c52e95632fc488dde6b1cf4R182
As parameterize function returns object I had to do this check.

Also renamed function from parametrize to parameterize. I think it would be the correct name.

And had to update to the latest prisma version as node integration tests weren't running locally (Ubuntu. 22.04.3)

… feature/added-parametrize-function

Lms24

Final pass - just a couple of small changes left then we're good to merge!

Lms24 · 2023-10-16T07:48:17Z

packages/utils/src/eventbuilder.ts

+  const { __sentry_template_string__, __sentry_template_values__ } = message;
+
+  if (isParameterizedString(message)) {


l: not a big deal but can we move the object deconstruction into the if block? I don't think we need it outside, right?

Lms24 · 2023-10-16T07:54:16Z

packages/browser/src/eventbuilder.ts

+  const { __sentry_template_string__, __sentry_template_values__ } = message;
+
+  if (isParameterizedString(message)) {


l: same here, let's move this into the if block

packages/utils/src/is.ts

Lms24 · 2023-10-16T08:09:30Z

Whoops, seems like integration tests aren't happy with the change:

Browser:

Not sure why they fail, can you take a look?

Node:

let's revert the prism upgrade. IIRC our prism integration isn't compatible with prism 5 (not 100% sure, though).

Btw, out of curiosity, can you see the test logs from CI?

AleshaOleg · 2023-11-30T19:55:54Z

@Lms24 by any chance if you have time, let's finish this up :) Seems like very little left to merge it. I would appreciate help with tests 🙏🏻 Can't figure out what's wrong with them when running in cloud 😢

…o feature/added-parametrize-function

Lms24 · 2023-12-01T12:47:49Z

Hey @AleshaOleg thanks for the ping! I currently don't have time to look at this but hopefully by the end of next week 🤞 . I set a reminder. Sorry for the inconvenience.

AleshaOleg · 2023-12-01T15:32:39Z

@Lms24 thanks! Just checked tests again. This one which is failing in the cloud, definitely running successfully locally.

Lms24

Hmm I gave this a quick look and what's interesting is that the integration tests fail for CDN bundles, not when using the npm package. Have you tried running integration tests locally for CDN bundles?

yarn build in the root of the repo (to build all CDN bundles)
In the browser integration tests directory, run test:bundle:tracing:es6:min for example (see package.json for more bundle variants)

AleshaOleg · 2023-12-01T15:46:49Z

Yes, I tried. As told couple of messages above, I'm usually getting just a timeout without any reason specifically for this test.

And after the timeout, other tests were also skipped. So, maybe you can try to run them locally.

Don't want to reduce timeout as on the cloud it will remain 30 seconds.

AleshaOleg · 2023-12-21T21:37:37Z

@Lms24 I think I found the problem. When parameterize function is being called here - https://github.com/getsentry/sentry-javascript/pull/9145/files#diff-c4a597fb313c930c8193b3d29797452db07042ea6f25da9664343002e47897d5R
something happens that stops it from executing and that's why the test throws a timeout error.

I'm currently thinking about what to do. As I don't know the cause of why this happens, I can now propose to rewrite the parameterize function without using the tag function. But in this case, I think, we will miss the whole point of why we're trying to implement it.

I'll try to figure out why this happens for bundle tests only, I think it's related obviously. Meanwhile, if you have any ideas please write them. Thanks!

into feature/added-parametrize-function

AleshaOleg · 2023-12-31T11:51:01Z

@Lms24 I think I figured out what's the problem with these tests, they're not running with any bundles except esm, cjs. Because of importing the parameterize function from outside. That's why the test didn't work for some bundle builds. This is fixed with skipping this test for bundle builds with help of shouldSkipTracingTest function here. Please run checks again and I think after that it's ready to be merged. Thanks!

into feature/added-parametrize-function

AleshaOleg · 2024-01-02T20:45:09Z

@Lms24 thanks, pushed one more commit. Please run tests again

Lms24

Hey @AleshaOleg thanks for still sticking with us! I appreciate your patience, unfortunately we're very busy with tasks at the moment, making external contribution reviews a bit of low-prio. That being said, I believe this PR is currently in a state where we can merge it without it causing any problems.

However, we eventually want to export this function from the SDK packages. This will also expose it on the Sentry.parameterize object in the CDN bundles which means at this point we should remove the skipping guard in the integration tests. Again, to get the fundamental functionality in at all, please don't do this in this PR but (if you still feel like contributing) do it in a follow up PR.

Gonna approve as soon as we sort out the skipping logic :)

Lms24 · 2024-01-03T10:25:00Z

...ges/browser-integration-tests/suites/public-api/captureMessage/parameterized_message/test.ts

+import { shouldSkipReplayTest } from '../../../../utils/replayHelpers';
+
+sentryTest('should capture a parameterized representation of the message', async ({ getLocalTestPath, page }) => {
+  if (shouldSkipReplayTest() || !shouldSkipTracingTest()) {


I think this is not gonna be enough. We need to skip all bundle_ variants for now. These two functions won't do that.

basically like in this test:

sentry-javascript/packages/browser-integration-tests/suites/replay/replayShim/test.ts

Lines 8 to 13 in 94bc349

const bundle = process.env.PW_BUNDLE;

if (!bundle || !bundle.startsWith('bundle_') || bundle.includes('replay')) {

sentryTest.skip();

}

Thanks for the hint about skipping the test for all bundle builds.
I made changes, should work now.

Can you also explain how you are testing such functions for public API for CDN bundles? Because importing functions directly to tests will not work, and I'm wondering how I can test them. Just an example would be enough, I didn't find anything similar in browser-integration-tests. Will do it as a separate PR as you mentioned

So as soon as we export parameterize from @sentry/browser, our CDN bundles will also include it as a top level function to call on the Sentry object. Meaning, CDN bundle users can call

Sentry.captureMessage(Sentry.parameterize`This is a log statement with ${x} and ${y} params`);

we can adjust the integration tests to do that and stop importing it from @sentry/utils.

that's generally how we test public API in the browser integration tests, across NPM package (esm, cjs) and CDN bundle variants.

For a simple example, take a look at this test: https://github.com/getsentry/sentry-javascript/blob/23ef22b115c8868861896cc9003bd4bb6afb0690/dev-packages/browser-integration-tests/suites/public-api/setTag/with_primitives/subject.js

calling Sentry.parameterize should work just like calling Sentry.setTag once we export it from the browser package.

Thanks, just tried also throws timeouts locally, but works with esm, cjs builds. Will do separate PR for that, and will mention you to run tests there, let's see if it will work on cloud setup. A lot of other tests also failing for me locally because of timeout for example for bundle_tracing_replay_es6_min, so I'm not quite sure if it works or not.

For now please merge it, just pulled latest changes from develop and I'll do separate PR for running tests for all of the builds and make this function available from browser API. Thanks!

into feature/added-parametrize-function

Lms24

Thanks @AleshaOleg! I'm gonna merge this in for now. Feel free to open the follow up PR whenever you have time.

Add `parameterize` fuction to utils package that takes params from a format string and attaches them as extra properties to a string. These are picked up during event processing and a `LogEntry` item is added to the event.

Added parametrize function

db66295

Lms24 reviewed Oct 2, 2023

View reviewed changes

Lms24 reviewed Oct 10, 2023

View reviewed changes

Populate logentry parameter if it's exist for eventFromMessage functions

e15d5f1

AleshaOleg requested a review from Lms24 October 11, 2023 20:10

Merge branch 'develop' of github.com:getsentry/sentry-javascript into…

7b27a1e

… feature/added-parametrize-function

AleshaOleg force-pushed the feature/added-parametrize-function branch from 6556d54 to 7b27a1e Compare October 11, 2023 20:13

Lms24 reviewed Oct 13, 2023

View reviewed changes

AleshaOleg force-pushed the feature/added-parametrize-function branch from d1dce8e to cdce21f Compare October 13, 2023 17:40

Renamed parametrize to parameterize and moved ParameterizedString typ…

e4c4d4c

…e to @sentry/types

AleshaOleg force-pushed the feature/added-parametrize-function branch from cdce21f to e4c4d4c Compare October 13, 2023 17:41

AleshaOleg added 3 commits October 14, 2023 01:05

Added integration tests

8fa0552

Fixing tests and improvements

d83411a

isParameterized -> isParameterizedString in docs

70e6864

AleshaOleg requested a review from Lms24 October 13, 2023 23:21

Fixed lint errors

cbd0805

AleshaOleg changed the title ~~Added parametrize function~~ Added parameterize function Oct 14, 2023

AleshaOleg added 2 commits October 14, 2023 20:27

Minor optimization

1020b14

Merge branch 'develop' of github.com:getsentry/sentry-javascript into…

99c4fbb

… feature/added-parametrize-function

Lms24 reviewed Oct 16, 2023

View reviewed changes

AleshaOleg added 2 commits October 16, 2023 19:04

Minor improvements, reverted prisma update

3cbe665

Fixed typo

de6e723

AleshaOleg added 2 commits November 30, 2023 23:13

Merge branch 'develop' of github.com:AleshaOleg/sentry-javascript int…

3e3b441

…o feature/added-parametrize-function

Fixed Biome errors

0d67997

Lms24 reviewed Dec 1, 2023

View reviewed changes

AleshaOleg added 3 commits December 21, 2023 23:04

Merge branch 'develop' of https://github.com/getsentry/sentry-javascript

dcd1549

into feature/added-parametrize-function

Merge branch 'develop' of https://github.com/getsentry/sentry-javascript

ca8a58a

into feature/added-parametrize-function

Skipping test for some bundles

8c8bf10

AleshaOleg requested a review from Lms24 December 31, 2023 15:10

AleshaOleg added 2 commits January 2, 2024 21:20

Prevent running test for tracing builds

a631c55

Merge branch 'develop' of https://github.com/getsentry/sentry-javascript

aa3c9d1

into feature/added-parametrize-function

Lms24 reviewed Jan 3, 2024

View reviewed changes

AleshaOleg added 3 commits January 4, 2024 01:38

Skipping all test for bundle builds

de11d01

Merge branch 'develop' of https://github.com/getsentry/sentry-javascript

5aeedf7

into feature/added-parametrize-function

Test migration to dev-packages folder

f68be1f

AleshaOleg requested a review from Lms24 January 4, 2024 00:51

AleshaOleg added 2 commits January 4, 2024 20:54

Merge branch 'develop' of https://github.com/getsentry/sentry-javascript

5681add

into feature/added-parametrize-function

Merge branch 'develop' into feature/added-parametrize-function

4b0c53c

Lms24 approved these changes Jan 5, 2024

View reviewed changes

Lms24 changed the title ~~Added parameterize function~~ feat(utils): Add parameterize function Jan 5, 2024

Lms24 merged commit 3545c68 into getsentry:develop Jan 5, 2024

AleshaOleg deleted the feature/added-parametrize-function branch January 5, 2024 13:28

AleshaOleg mentioned this pull request Jan 6, 2024

feat(utils): Make parameterize function available through browser and node API #10085

Merged

2 tasks

AleshaOleg mentioned this pull request Jan 23, 2024

Add helper function for interpolating logging statements #6725

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(utils): Add `parameterize` function #9145

feat(utils): Add `parameterize` function #9145

AleshaOleg commented Sep 29, 2023

Lms24 left a comment

AleshaOleg commented Oct 2, 2023

AleshaOleg commented Oct 5, 2023

Lms24 commented Oct 6, 2023

Lms24 left a comment •

edited

Loading

AleshaOleg commented Oct 10, 2023

Lms24 commented Oct 10, 2023 •

edited

Loading

AleshaOleg commented Oct 11, 2023

Lms24 left a comment

Lms24 Oct 13, 2023

AleshaOleg Oct 13, 2023

AleshaOleg commented Oct 13, 2023 •

edited

Loading

Lms24 left a comment

Lms24 Oct 16, 2023

Lms24 Oct 16, 2023

Lms24 commented Oct 16, 2023 •

edited

Loading

AleshaOleg commented Nov 30, 2023

Lms24 commented Dec 1, 2023

AleshaOleg commented Dec 1, 2023

Lms24 left a comment

AleshaOleg commented Dec 1, 2023

AleshaOleg commented Dec 21, 2023

AleshaOleg commented Dec 31, 2023 •

edited

Loading

AleshaOleg commented Jan 2, 2024

Lms24 left a comment

Lms24 Jan 3, 2024

AleshaOleg Jan 4, 2024

Lms24 Jan 4, 2024

Lms24 Jan 4, 2024

Lms24 Jan 4, 2024

AleshaOleg Jan 4, 2024 •

edited

Loading

Lms24 left a comment

	/**
	* @hidden
	*/
	export function eventFromString(
	stackParser: StackParser,
	input: string,
	syntheticException?: Error,
	attachStacktrace?: boolean,
	): Event {
	const event: Event = {
	message: input,
	};

	if (attachStacktrace && syntheticException) {
	const frames = parseStackFrames(stackParser, syntheticException);
	if (frames.length) {
	event.exception = {
	values: [{ value: input, stacktrace: { frames } }],
	};
	}
	}

	return event;
	}

	/** Creates an {@link Event} from primitive inputs to `captureMessage`. */
	eventFromMessage(
	message: string,
	// eslint-disable-next-line deprecation/deprecation
	level?: Severity \| SeverityLevel,
	hint?: EventHint,
	): PromiseLike<Event>;

		const { __sentry_template_string__, __sentry_template_values__ } = message;

		if (isParameterizedString(message)) {

	const bundle = process.env.PW_BUNDLE;

	if (!bundle \|\| !bundle.startsWith('bundle_') \|\| bundle.includes('replay')) {
	sentryTest.skip();
	}

feat(utils): Add parameterize function #9145

feat(utils): Add parameterize function #9145

Conversation

AleshaOleg commented Sep 29, 2023

Lms24 left a comment

Choose a reason for hiding this comment

AleshaOleg commented Oct 2, 2023

AleshaOleg commented Oct 5, 2023

Lms24 commented Oct 6, 2023

Lms24 left a comment • edited Loading

Choose a reason for hiding this comment

AleshaOleg commented Oct 10, 2023

Lms24 commented Oct 10, 2023 • edited Loading

AleshaOleg commented Oct 11, 2023

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AleshaOleg commented Oct 13, 2023 • edited Loading

Lms24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lms24 commented Oct 16, 2023 • edited Loading

AleshaOleg commented Nov 30, 2023

Lms24 commented Dec 1, 2023

AleshaOleg commented Dec 1, 2023

Lms24 left a comment

Choose a reason for hiding this comment

AleshaOleg commented Dec 1, 2023

AleshaOleg commented Dec 21, 2023

AleshaOleg commented Dec 31, 2023 • edited Loading

AleshaOleg commented Jan 2, 2024

Lms24 left a 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

AleshaOleg Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Lms24 left a comment

Choose a reason for hiding this comment

feat(utils): Add `parameterize` function #9145

feat(utils): Add `parameterize` function #9145

Lms24 left a comment •

edited

Loading

Lms24 commented Oct 10, 2023 •

edited

Loading

AleshaOleg commented Oct 13, 2023 •

edited

Loading

Lms24 commented Oct 16, 2023 •

edited

Loading

AleshaOleg commented Dec 31, 2023 •

edited

Loading

AleshaOleg Jan 4, 2024 •

edited

Loading