feat(utils): Add parameterize function

[AleshaOleg]

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]

@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]

@Lms24 any update would be appreciated :)

[Lms24]

@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]

@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]

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.

[AleshaOleg]

@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:

• node: https://github.com/getsentry/sentry-javascript/tree/lms/astro-integration-keywords/packages/node-integration-tests/suites/public-api/captureMessage
• browser: https://github.com/getsentry/sentry-javascript/tree/lms/astro-integration-keywords/packages/browser-integration-tests/suites/public-api/captureMessage

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]

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

[AleshaOleg]

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

[AleshaOleg]

@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)

[Lms24]

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

[Lms24]

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]

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

[Lms24]

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]

@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 😢

[Lms24]

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]

@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?

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

[AleshaOleg]

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]

@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!

[AleshaOleg]

@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!

[AleshaOleg]

@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]

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();
	}

[AleshaOleg]

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

[Lms24]

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.

[Lms24]

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

[Lms24]

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.

[AleshaOleg]

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!

[Lms24]

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