Skip to content

feat(profiling): Detect chunks without client sdk #89195

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

Merged
merged 3 commits into from
Apr 15, 2025
Merged

feat(profiling): Detect chunks without client sdk #89195

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
93c73a3
feat(profiling): Detect chunks without client sdk
Zylphrex Apr 9, 2025
ba8816d
Merge branch 'master' into txiao/feat/detect-chunks-without-client-sdk
Zylphrex Apr 14, 2025
3c8033a
address PR comments
Zylphrex Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/sentry/profiles/task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1137,19 +1137,38 @@ def determine_profile_type(profile: Profile) -> EventType:
raise UnknownProfileTypeException


def track_latest_sdk(project: Project, profile: Profile) -> None:
event_type = determine_profile_type(profile)

def determine_client_sdk(profile: Profile, event_type: EventType) -> tuple[str, str]:
client_sdk = profile.get("client_sdk")

if not client_sdk:
raise UnknownClientSDKException
if client_sdk:
sdk_name = client_sdk.get("name")
sdk_version = client_sdk.get("version")

if sdk_name and sdk_version:
return sdk_name, sdk_version

# some older sdks were sending the profile chunk without the
# sdk info, here we hard code a few and assign them a guaranteed
# outdated version
if event_type == EventType.PROFILE_CHUNK:
if profile["platform"] == "python":
return "sentry.python", "0.0.0"
elif profile["platform"] == "cocoa":
return "sentry.cocoa", "0.0.0"
elif profile["platform"] == "node":
# there are other node platforms but it's not straight forward
# to figure out which it is here so collapse them all into just node
Comment on lines +1159 to +1160
Copy link
Contributor

@viglia viglia Apr 11, 2025

Choose a reason for hiding this comment

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

For Android we don't get this issue because the SDK was released recently and it set the client_sdk since the beginning, right?

Copy link
Member Author

@Zylphrex Zylphrex Apr 11, 2025

Choose a reason for hiding this comment

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

@stefanosiano do you have the answer to this?

Copy link
Member

@stefanosiano stefanosiano Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

sdk name and versions are set in the envelope headers by default in the Android SDK since the beginning
v 8.6.0 (ui profiling is officially out in 8.7.0) added platform in chunk header

Copy link
Member Author

@Zylphrex Zylphrex Apr 11, 2025

Choose a reason for hiding this comment

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

Can you elaborate on

sdk name and versions are set in the envelope headers by default
Profiles are expecting the sdk info within the envelope item so if it's in the header, we'll need to handle that somehow.

And the difference between v8.6.0 and v8.7.0 is just that additional platform in the chunk header right?

Copy link
Member

@stefanosiano stefanosiano Apr 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

android envelopes (not the chunk item) contains sdk header, with name and version

{
  ...,
  "sdk": { 
    "name":"sentry.java.android",
    "version":"8.7.0",
    ...
  },
  ...

And the difference between v8.6.0 and v8.7.0 is just that additional platform in the chunk header right?

8.6.0 already has the platform in the chunk header. 8.7.0 moves all the ui profiling options out of experimental and implement last things - avoid stopping profiles immediately (it waits for current chunk to finish) and stops profile when app goes to background

Copy link
Member Author

@Zylphrex Zylphrex Apr 11, 2025

Choose a reason for hiding this comment

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

Okay, so

  1. we'll need to handle the sdk info coming from the header
  2. the payload sent by 8.6.0 and 8.7.0 are identical

Copy link
Contributor

@viglia viglia Apr 11, 2025

Choose a reason for hiding this comment

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

Okay, so

  1. we'll need to handle the sdk info coming from the header
  2. the payload sent by 8.6.0 and 8.7.0 are identical

Ok, this something we'll handle in Relay, which means this PR is good to go?
Maybe I'd just add a comment in here explaining why for android we don't need to assign an outdated version.

Copy link
Member Author

@Zylphrex Zylphrex Apr 14, 2025

Choose a reason for hiding this comment

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

added in 3c8033a

return "sentry.javascript.node", "0.0.0"

sdk_name = client_sdk.get("name")
sdk_version = client_sdk.get("version")
# Other platforms do not have a version released where it sends
# a profile chunk without the client sdk info

if not sdk_name or not sdk_version:
raise UnknownClientSDKException
raise UnknownClientSDKException


def track_latest_sdk(project: Project, profile: Profile) -> None:
event_type = determine_profile_type(profile)
sdk_name, sdk_version = determine_client_sdk(profile, event_type)

try:
ProjectSDK.update_with_newest_version_or_create(
Expand Down
48 changes: 48 additions & 0 deletions tests/sentry/profiles/test_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -977,3 +977,51 @@ def test_track_latest_sdk(
)
is not None
)


@patch("sentry.profiles.task._push_profile_to_vroom")
@patch("sentry.profiles.task._symbolicate_profile")
@patch("sentry.models.projectsdk.get_sdk_index")
@pytest.mark.parametrize(
["platform", "sdk_name"],
[
("python", "sentry.python"),
("cocoa", "sentry.cocoa"),
("node", "sentry.javascript.node"),
],
)
@django_db_all
def test_unknown_sdk(
get_sdk_index,
_symbolicate_profile,
_push_profile_to_vroom,
platform,
sdk_name,
organization,
project,
request,
):
_push_profile_to_vroom.return_value = True
_symbolicate_profile.return_value = True
get_sdk_index.return_value = {
sdk_name: {},
}

profile = request.getfixturevalue("sample_v2_profile")
profile["organization_id"] = organization.id
profile["project_id"] = project.id
profile["platform"] = platform
del profile["client_sdk"]

with Feature("organizations:profiling-sdks"):
process_profile_task(profile=profile)

assert (
ProjectSDK.objects.get(
project=project,
event_type=EventType.PROFILE_CHUNK.value,
sdk_name=sdk_name,
sdk_version="0.0.0",
)
is not None
)
Loading