-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
feat(vercel): Add drain env variables to Vercel integration #103986
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
feat(vercel): Add drain env variables to Vercel integration #103986
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #103986 +/- ##
===========================================
+ Coverage 76.12% 80.61% +4.49%
===========================================
Files 9311 9313 +2
Lines 397260 397439 +179
Branches 25356 25356
===========================================
+ Hits 302403 320404 +18001
+ Misses 94416 76594 -17822
Partials 441 441 |
| .with_project(sentry_project) | ||
| .with_project_key(project_key) | ||
| .with_auth_token(sentry_auth_token) | ||
| .with_framework(vercel_project.get("framework")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Framework validation fails when Vercel project lacks framework
The builder calls with_framework(vercel_project.get("framework")) which passes None when a Vercel project lacks a framework field. However, the build() method raises ValueError("framework is required") when framework is None. This breaks integration setup for Vercel projects without a detected framework, which is a valid scenario. The old implementation expected a string framework parameter, but the new code doesn't handle the missing framework case.
Additional Locations (1)
vgrozdanic
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, i just wouldn't raise the error if the framework is None
| if self._framework is None: | ||
| raise ValueError("framework is required") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the framework really required here?
From what i could tell it is only important to know if it is Next.js or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah you're right - fixed with 048a857
Now that getsentry/getsentry#18932 and #103986 has merged, we can remove the `get_env_var_map` as you should now just use the `VercelEnvVarMapBuilder` class instead.
ref https://linear.app/getsentry/issue/LOGS-516/add-vercel-drain-urls-as-environmental-variables-to-vercel-integration
As part of our work to add on-click install drains for Vercel, I'm updating the Vercel integration to add three new environmental variables.
SENTRY_VERCEL_LOG_DRAIN_URL: The Vercel log drain URLSENTRY_OTLP_TRACES_URL: The OTLP traces URL that is used for vercel trace drainsSENTRY_PUBLIC_KEY: The public key that is used to authenticate with sentryTo get an idea of how these are used, check out the vercel drain docs: https://docs.sentry.io/product/drains/integration/vercel/
Previously we were populating these environmental variables with the
get_env_var_maphelper function. This is also used in thegetsentrycodebase. As this was difficult to manage, I replaced it with aVercelEnvVarMapBuilderbuilder class, that offers us more extensibility in case we need to add more environmental variables in the future. After we've updated theget_env_var_mapusage in thegetsentrycodebase, we can remove it here as well.To make the new environmental variables work with rpcs, I've had to serialize two new variables into
RpcProjectKey. These arepublic_key, used to populateSENTRY_PUBLIC_KEYandintegration_endpoint, used to populate bothSENTRY_VERCEL_LOG_DRAIN_URLandSENTRY_OTLP_TRACES_URL.I'm still not sure about the naming for
SENTRY_PUBLIC_KEY, so open to any feedback there for that.