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.

Sign up for GitHub

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

Jump to bottom

feat(crons): Pass-through minimal sdk field #2073

Merged
merged 1 commit into from
Apr 28, 2023
Merged

feat(crons): Pass-through minimal sdk field #2073

merged 1 commit into from
Apr 28, 2023

Conversation

evanpurkhiser
Copy link
Member

@evanpurkhiser evanpurkhiser commented Apr 27, 2023 •
edited by jan-auer
Loading

We would like to collect more information about where cron checkins are coming from. Since we're not using metastructure yet we can't use the ClientSdkInfo struct

relay/relay-general/src/protocol/clientsdk.rs

Lines 18 to 65 in 3ffdaf1

pub struct ClientSdkInfo {
/// Unique SDK name. _Required._
///
/// The name of the SDK. The format is `entity.ecosystem[.flavor]` where entity identifies the
/// developer of the SDK, ecosystem refers to the programming language or platform where the
/// SDK is to be used and the optional flavor is used to identify standalone SDKs that are part
/// of a major ecosystem.
///
/// Official Sentry SDKs use the entity `sentry`, as in `sentry.python` or
/// `sentry.javascript.react-native`. Please use a different entity for your own SDKs.
#[metastructure(required = "true", max_chars = "symbol")]
pub name: Annotated<String>,
/// The version of the SDK. _Required._
///
/// It should have the [Semantic Versioning](https://semver.org/) format `MAJOR.MINOR.PATCH`,
/// without any prefix (no `v` or anything else in front of the major version number).
///
/// Examples: `0.1.0`, `1.0.0`, `4.3.12`
#[metastructure(required = "true", max_chars = "symbol")]
pub version: Annotated<String>,
/// List of integrations that are enabled in the SDK. _Optional._
///
/// The list should have all enabled integrations, including default integrations. Default
/// integrations are included because different SDK releases may contain different default
/// integrations.
#[metastructure(skip_serialization = "empty_deep")]
pub integrations: Annotated<Array<String>>,
/// List of installed and loaded SDK packages. _Optional._
///
/// A list of packages that were installed as part of this SDK or the activated integrations.
/// Each package consists of a name in the format `source:identifier` and `version`. If the
/// source is a Git repository, the `source` should be `git`, the identifier should be a
/// checkout link and the version should be a Git reference (branch, tag or SHA).
#[metastructure(skip_serialization = "empty_deep")]
pub packages: Annotated<Array<ClientSdkPackage>>,
/// IP Address of sender??? Seems unused. Do not send, this only leads to surprises wrt PII, as
/// the value appears nowhere in the UI.
#[metastructure(pii = "true", skip_serialization = "empty", omit_from_schema)]
pub client_ip: Annotated<IpAddr>,
/// Additional arbitrary fields for forwards compatibility.
#[metastructure(additional_properties)]
pub other: Object<Value>,
}

#skip-changelog

@evanpurkhiser evanpurkhiser requested a review from a team April 27, 2023 00:36
jjbayer
jjbayer requested changes Apr 27, 2023
View reviewed changes
relay-monitors/src/lib.rs Show resolved Hide resolved
relay-monitors/src/lib.rs Show resolved Hide resolved
relay-monitors/src/lib.rs Outdated
@@ -113,6 +124,9 @@ struct CheckIn {
/// Status of this check-in. Defaults to `"unknown"`.
status: CheckInStatus,

/// monitor configuration to support upserts.
sdk: MinimalClientSdkInfo,
Copy link
Member

Choose a reason for hiding this comment

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

Old SDKs won't send this, right? So I think this needs to be optional, otherwise deserialization will fail. Make sure to also add a test case to verify this.

Suggested change
sdk: MinimalClientSdkInfo,
#[serde(default, skip_serializing_if = "Option::is_none")]
sdk: Option<MinimalClientSdkInfo>,

Copy link
Member Author

Choose a reason for hiding this comment

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

Old SDKs don’t support sending monitor checkins however

Copy link
Member

Choose a reason for hiding this comment

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

Did we not release an SDK version already with this? If not we can skip this, but if we did, we should make this optional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Chatted w/ @jan-auer I'll go ahead and make this optional

@rjo100 rjo100 mentioned this pull request Apr 27, 2023
Closed
@evanpurkhiser evanpurkhiser force-pushed the evanpurkhiser/feat-crons-pass-through-minimal-sdk-field branch from 2ae7a6f to bd46d80 Compare April 27, 2023 22:00
jan-auer
jan-auer approved these changes Apr 27, 2023
View reviewed changes
relay-monitors/src/lib.rs Show resolved Hide resolved
relay-monitors/src/lib.rs Outdated
@@ -113,6 +124,9 @@ struct CheckIn {
/// Status of this check-in. Defaults to `"unknown"`.
status: CheckInStatus,

/// monitor configuration to support upserts.
sdk: MinimalClientSdkInfo,
Copy link
Member

Choose a reason for hiding this comment

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

Did we not release an SDK version already with this? If not we can skip this, but if we did, we should make this optional.

@evanpurkhiser evanpurkhiser merged commit 7539958 into master Apr 28, 2023
@evanpurkhiser evanpurkhiser deleted the evanpurkhiser/feat-crons-pass-through-minimal-sdk-field branch April 28, 2023 18:21
jan-auer added a commit that referenced this pull request May 2, 2023
@jan-auer
Merge branch 'master' into build/jemalloc
cb3dfb6 
* master:
  ci(gha): add debugging for "Couldn'\''t write tracker file" issue (#2081)
  feat(crons): Pass-through minimal `sdk` field (#2073)
  fix(quota): Parse large limits (#2079)
  instr(server): More details on unexpected envelope drop (#2077)
  fix(ci): Skip flaky integration test (#2076)
jan-auer added a commit that referenced this pull request May 9, 2023
@jan-auer
Merge branch 'master' into remove-unsupported-macos-15
c974258 
* master: (178 commits)
  ref(crons): Allow `_` in monitor slugs (#2100)
  fix(span-metrics): Scrub subdomains in metric tags (#2103)
  feat(normalization): Scrub span description URLs (#2095)
  feat(dynamic-sampling): Add possibility to run dynamic sampling from `sentry-relay` (#2091)
  ref(dynamic-config): Remove transaction metrics allowlist from Sentry (#2092)
  feat(protocol): Support old effective-directory format (#2048)
  ref(spans): Add more tags to span metrics (#2090)
  fix(server): Support HTTP half-close connections (#2089)
  ref(spans): Use transactions namespace with `span` prefix (#2087)
  ref(spans): Update span metrics feature flag name (#2086)
  feat(metrics): Extract more metrics from spans (#2080)
  build: Bump tempfile to remove flagged dependency (#2085)
  feat(filter): Add document_uri csp filter (#2059)
  build: Use jemalloc on Linux (#2084)
  ci(gha): add debugging for "Couldn'\''t write tracker file" issue (#2081)
  feat(crons): Pass-through minimal `sdk` field (#2073)
  fix(quota): Parse large limits (#2079)
  instr(server): More details on unexpected envelope drop (#2077)
  fix(ci): Skip flaky integration test (#2076)
  feat(dynamic-sampling): Add support for `trace.replay_id` in matching rules (#2070)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjbayer jjbayer jjbayer approved these changes

@jan-auer jan-auer jan-auer approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evanpurkhiser @jan-auer @jjbayer