From e65de540eec5b21aa82aac52473c6a80afdc885e Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Thu, 3 Aug 2023 15:23:57 +0200 Subject: [PATCH 1/9] use profiles that were not dynamically sampled to enhance slowest functions aggregation --- cmd/vroom/profile.go | 126 ++++++++++++++++-------------- internal/profile/legacy.go | 5 ++ internal/profile/profile.go | 5 ++ internal/sample/sample.go | 5 ++ internal/speedscope/speedscope.go | 1 + 5 files changed, 82 insertions(+), 60 deletions(-) diff --git a/cmd/vroom/profile.go b/cmd/vroom/profile.go index 9423b2f..eb8f0c5 100644 --- a/cmd/vroom/profile.go +++ b/cmd/vroom/profile.go @@ -69,21 +69,23 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { p.Normalize() s.Finish() - s = sentry.StartSpan(ctx, "gcs.write") - s.Description = "Write profile to GCS" - err = storageutil.CompressedWrite(ctx, env.storage, p.StoragePath(), p) - s.Finish() - if err != nil { - var e *googleapi.Error - if ok := errors.As(err, &e); ok { - w.WriteHeader(http.StatusBadGateway) - } else if errors.Is(err, context.DeadlineExceeded) { - w.WriteHeader(http.StatusTooManyRequests) - } else { - hub.CaptureException(err) - w.WriteHeader(http.StatusInternalServerError) + if p.IsSampled() { + s = sentry.StartSpan(ctx, "gcs.write") + s.Description = "Write profile to GCS" + err = storageutil.CompressedWrite(ctx, env.storage, p.StoragePath(), p) + s.Finish() + if err != nil { + var e *googleapi.Error + if ok := errors.As(err, &e); ok { + w.WriteHeader(http.StatusBadGateway) + } else if errors.Is(err, context.DeadlineExceeded) { + w.WriteHeader(http.StatusTooManyRequests) + } else { + hub.CaptureException(err) + w.WriteHeader(http.StatusInternalServerError) + } + return } - return } s = sentry.StartSpan(ctx, "processing") @@ -97,35 +99,37 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { } if len(callTrees) > 0 { - s = sentry.StartSpan(ctx, "processing") - s.Description = "Find occurrences" - occurrences := occurrence.Find(p, callTrees) - s.Finish() + if p.IsSampled() { + s = sentry.StartSpan(ctx, "processing") + s.Description = "Find occurrences" + occurrences := occurrence.Find(p, callTrees) + s.Finish() - // Filter in-place occurrences without a type. - var i int - for _, o := range occurrences { - if o.Type != occurrence.NoneType { - occurrences[i] = o - i++ + // Filter in-place occurrences without a type. + var i int + for _, o := range occurrences { + if o.Type != occurrence.NoneType { + occurrences[i] = o + i++ + } } - } - occurrences = occurrences[:i] - s = sentry.StartSpan(ctx, "processing") - s.Description = "Build Kafka message batch" - occurrenceMessages, err := occurrence.GenerateKafkaMessageBatch(occurrences) - s.Finish() - if err != nil { - // Report the error but don't fail profile insertion - hub.CaptureException(err) - } else { + occurrences = occurrences[:i] s = sentry.StartSpan(ctx, "processing") - s.Description = "Send occurrences to Kafka" - err = env.occurrencesWriter.WriteMessages(ctx, occurrenceMessages...) + s.Description = "Build Kafka message batch" + occurrenceMessages, err := occurrence.GenerateKafkaMessageBatch(occurrences) s.Finish() if err != nil { // Report the error but don't fail profile insertion hub.CaptureException(err) + } else { + s = sentry.StartSpan(ctx, "processing") + s.Description = "Send occurrences to Kafka" + err = env.occurrencesWriter.WriteMessages(ctx, occurrenceMessages...) + s.Finish() + if err != nil { + // Report the error but don't fail profile insertion + hub.CaptureException(err) + } } } @@ -159,31 +163,33 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { } } - // Prepare profile Kafka message - s = sentry.StartSpan(ctx, "processing") - s.Description = "Marshal profile metadata Kafka message" - b, err := json.Marshal(buildProfileKafkaMessage(p)) - s.Finish() - if err != nil { - hub.CaptureException(err) - w.WriteHeader(http.StatusInternalServerError) - return - } + if p.IsSampled() { + // Prepare profile Kafka message + s = sentry.StartSpan(ctx, "processing") + s.Description = "Marshal profile metadata Kafka message" + b, err := json.Marshal(buildProfileKafkaMessage(p)) + s.Finish() + if err != nil { + hub.CaptureException(err) + w.WriteHeader(http.StatusInternalServerError) + return + } - s = sentry.StartSpan(ctx, "processing") - s.Description = "Send profile metadata to Kafka" - err = env.profilingWriter.WriteMessages(ctx, kafka.Message{ - Topic: env.config.ProfilesKafkaTopic, - Value: b, - }) - s.Finish() - hub.Scope().SetContext("Profile metadata Kafka payload", map[string]interface{}{ - "Size": len(b), - }) - if err != nil { - hub.CaptureException(err) - w.WriteHeader(http.StatusInternalServerError) - return + s = sentry.StartSpan(ctx, "processing") + s.Description = "Send profile metadata to Kafka" + err = env.profilingWriter.WriteMessages(ctx, kafka.Message{ + Topic: env.config.ProfilesKafkaTopic, + Value: b, + }) + s.Finish() + hub.Scope().SetContext("Profile metadata Kafka payload", map[string]interface{}{ + "Size": len(b), + }) + if err != nil { + hub.CaptureException(err) + w.WriteHeader(http.StatusInternalServerError) + return + } } w.WriteHeader(http.StatusNoContent) diff --git a/internal/profile/legacy.go b/internal/profile/legacy.go index 35d4e6d..dbb00a0 100644 --- a/internal/profile/legacy.go +++ b/internal/profile/legacy.go @@ -28,6 +28,7 @@ type ( } RawProfile struct { + Sampled bool `json:"sampled"` AndroidAPILevel uint32 `json:"android_api_level,omitempty"` Architecture string `json:"architecture,omitempty"` BuildID string `json:"build_id,omitempty"` @@ -258,3 +259,7 @@ func (p LegacyProfile) GetTransactionMetadata() transaction.Metadata { func (p LegacyProfile) GetTransactionTags() map[string]string { return p.TransactionTags } + +func (p LegacyProfile) IsSampled() bool { + return p.Sampled +} diff --git a/internal/profile/profile.go b/internal/profile/profile.go index ff8364b..2503bff 100644 --- a/internal/profile/profile.go +++ b/internal/profile/profile.go @@ -36,6 +36,7 @@ type ( Normalize() Speedscope() (speedscope.Output, error) StoragePath() string + IsSampled() bool } Profile struct { @@ -151,3 +152,7 @@ func (p *Profile) TransactionMetadata() transaction.Metadata { func (p *Profile) TransactionTags() map[string]string { return p.profile.GetTransactionTags() } + +func (p *Profile) IsSampled() bool { + return p.profile.IsSampled() +} diff --git a/internal/sample/sample.go b/internal/sample/sample.go index f0da866..af5b72d 100644 --- a/internal/sample/sample.go +++ b/internal/sample/sample.go @@ -78,6 +78,7 @@ type ( } RawProfile struct { + Sampled bool `json:"sampled"` DebugMeta debugmeta.DebugMeta `json:"debug_meta"` Device Device `json:"device"` Environment string `json:"environment,omitempty"` @@ -642,3 +643,7 @@ func (p RawProfile) GetTransactionMetadata() transaction.Metadata { func (p RawProfile) GetTransactionTags() map[string]string { return p.TransactionTags } + +func (p RawProfile) IsSampled() bool { + return p.Sampled +} diff --git a/internal/speedscope/speedscope.go b/internal/speedscope/speedscope.go index 3fc59af..a9f582d 100644 --- a/internal/speedscope/speedscope.go +++ b/internal/speedscope/speedscope.go @@ -106,6 +106,7 @@ type ( } ProfileView struct { + Sampled bool `json:"sampled"` //nolint:unused AndroidAPILevel uint32 `json:"androidAPILevel,omitempty"` //nolint:unused Architecture string `json:"architecture,omitempty"` //nolint:unused BuildID string `json:"-"` //nolint:unused From ce8f64c82e7d258716f60b510d70841230cc8a5e Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Thu, 3 Aug 2023 15:30:24 +0200 Subject: [PATCH 2/9] Add description to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 60099c5..f051cd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ **Internal** - Change function fingerprints to uint32. ([#295](https://github.com/getsentry/vroom/pull/295)) +- Use non dynamically sampled profiles to enhance slowest functions aggregation with more data ([300](https://github.com/getsentry/vroom/pull/300)) ## 23.7.1 From dbccbbe812ba931815275e16c1055c65256c3c5b Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Thu, 3 Aug 2023 15:39:06 +0200 Subject: [PATCH 3/9] Changelog period --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f051cd8..0731d92 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ **Internal** - Change function fingerprints to uint32. ([#295](https://github.com/getsentry/vroom/pull/295)) -- Use non dynamically sampled profiles to enhance slowest functions aggregation with more data ([300](https://github.com/getsentry/vroom/pull/300)) +- Use non dynamically sampled profiles to enhance slowest functions aggregation with more data. ([300](https://github.com/getsentry/vroom/pull/300)) ## 23.7.1 From c4a1ffdd9fa5243e0ae0150d96d0753f99af8ef3 Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Thu, 3 Aug 2023 15:45:47 +0200 Subject: [PATCH 4/9] Add hash to PR number in the changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0731d92..047cebb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ **Internal** - Change function fingerprints to uint32. ([#295](https://github.com/getsentry/vroom/pull/295)) -- Use non dynamically sampled profiles to enhance slowest functions aggregation with more data. ([300](https://github.com/getsentry/vroom/pull/300)) +- Use non dynamically sampled profiles to enhance slowest functions aggregation with more data. ([#300](https://github.com/getsentry/vroom/pull/300)) ## 23.7.1 From 5205ee704b2d079112038001dbfbe23522da428d Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Wed, 17 Jan 2024 15:10:56 +0100 Subject: [PATCH 5/9] Assign default profile ID to unsampled profiles --- cmd/vroom/profile.go | 9 +++++++++ internal/profile/legacy.go | 4 ++++ internal/profile/profile.go | 5 +++++ internal/sample/sample.go | 4 ++++ 4 files changed, 22 insertions(+) diff --git a/cmd/vroom/profile.go b/cmd/vroom/profile.go index f2faee3..2f34873 100644 --- a/cmd/vroom/profile.go +++ b/cmd/vroom/profile.go @@ -26,6 +26,7 @@ import ( const maxUniqueFunctionsPerProfile = 100 func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { + const unsampledProfileID = "00000000000000000000000000000000" ctx := r.Context() hub := sentry.GetHubFromContext(ctx) @@ -71,6 +72,14 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { p.Normalize() s.Finish() + if !p.IsSampled() { + // if we're dealing with an unsampled profile + // we'll assign the special "000....00" profile ID + // so that we can handle it accordingly either in + // either of snuba/sentry/front-end + p.SetProfileID(unsampledProfileID) + } + if p.IsSampled() { s = sentry.StartSpan(ctx, "gcs.write") s.Description = "Write profile to GCS" diff --git a/internal/profile/legacy.go b/internal/profile/legacy.go index db4fb18..87f0137 100644 --- a/internal/profile/legacy.go +++ b/internal/profile/legacy.go @@ -298,6 +298,10 @@ func (p LegacyProfile) GetMeasurements() map[string]measurements.Measurement { return p.Measurements } +func (p *LegacyProfile) SetProfileID(ID string) { + p.ProfileID = ID +} + // This is to be used for ReactNative JS profile only since it works based on the // assumption that we'll only have 1 thread in the JS profile, as is the case // for ReactNative. diff --git a/internal/profile/profile.go b/internal/profile/profile.go index fa86e04..d1278a1 100644 --- a/internal/profile/profile.go +++ b/internal/profile/profile.go @@ -39,6 +39,7 @@ type ( Speedscope() (speedscope.Output, error) StoragePath() string IsSampled() bool + SetProfileID(ID string) } Profile struct { @@ -159,6 +160,10 @@ func (p *Profile) IsSampled() bool { return p.profile.IsSampled() } +func (p *Profile) SetProfileID(ID string) { + p.profile.SetProfileID(ID) +} + func (p *Profile) Measurements() map[string]measurements.Measurement { return p.profile.GetMeasurements() } diff --git a/internal/sample/sample.go b/internal/sample/sample.go index 6a2409f..9c5f685 100644 --- a/internal/sample/sample.go +++ b/internal/sample/sample.go @@ -662,3 +662,7 @@ func (p RawProfile) IsSampled() bool { func (p RawProfile) GetMeasurements() map[string]measurements.Measurement { return p.Measurements } + +func (p *RawProfile) SetProfileID(ID string) { + p.EventID = ID +} From 9c8d6d1cbb091918e473ed7d8a4cecc24c686ebb Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Wed, 17 Jan 2024 15:12:45 +0100 Subject: [PATCH 6/9] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 11a1125..37db364 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ **Features**: - Add support for speedscope rendering of Android reactnative profiles ([#386](https://github.com/getsentry/vroom/pull/386)) +- Use profiles that were not dynamically sampled to enhance slowest functions aggregation ([#300](https://github.com/getsentry/vroom/pull/300)) **Bug Fixes**: From 807239fe7b8fb6138229c0d00d403ef5bef3f584 Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Tue, 23 Jan 2024 17:18:46 +0100 Subject: [PATCH 7/9] move constant definition outside of function body --- cmd/vroom/profile.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/vroom/profile.go b/cmd/vroom/profile.go index 2f34873..34b63fa 100644 --- a/cmd/vroom/profile.go +++ b/cmd/vroom/profile.go @@ -23,10 +23,12 @@ import ( "github.com/getsentry/vroom/internal/storageutil" ) -const maxUniqueFunctionsPerProfile = 100 +const ( + maxUniqueFunctionsPerProfile = 100 + unsampledProfileID = "00000000000000000000000000000000" +) func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { - const unsampledProfileID = "00000000000000000000000000000000" ctx := r.Context() hub := sentry.GetHubFromContext(ctx) From de0314a5a046cb837d441d607de2aa9f33c7267b Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Tue, 23 Jan 2024 17:24:56 +0100 Subject: [PATCH 8/9] Add a comment to the p.IsSampled() check --- cmd/vroom/profile.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/vroom/profile.go b/cmd/vroom/profile.go index 34b63fa..eedc1e2 100644 --- a/cmd/vroom/profile.go +++ b/cmd/vroom/profile.go @@ -114,6 +114,9 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { } if len(callTrees) > 0 { + // if the profile was not sampled we skip find_occurrences since we're only + // interested in extracting data to improve functions aggregations not in + // using it for finding occurrences of an issue if p.IsSampled() { s = sentry.StartSpan(ctx, "processing") s.Description = "Find occurrences" From 33d36c38833a9310aedd96b831b1d495536687a7 Mon Sep 17 00:00:00 2001 From: Francesco Vigliaturo Date: Tue, 23 Jan 2024 18:02:36 +0100 Subject: [PATCH 9/9] Return early if writing to gcs error out --- cmd/vroom/profile.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/vroom/profile.go b/cmd/vroom/profile.go index eedc1e2..6b7498d 100644 --- a/cmd/vroom/profile.go +++ b/cmd/vroom/profile.go @@ -100,6 +100,7 @@ func (env *environment) postProfile(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusInternalServerError) } } + return } }