Skip to content

feat(core): Add metric summaries to spans - #10432

Merged
AbhiPrasad merged 12 commits into
getsentry:developfrom
timfish:feat/metric-summaries
Feb 5, 2024
Merged

feat(core): Add metric summaries to spans#10432
AbhiPrasad merged 12 commits into
getsentry:developfrom
timfish:feat/metric-summaries

Conversation

@timfish

@timfishtimfish commented Jan 30, 2024•
edited
Loading

Copy link
Copy Markdown
Collaborator

A quick PoC for #10059

I read the RFC, but it's almost entirely inspired by the Python implementation.

Outstanding issues:

  • Summaries are only attached to spans, should they also be attached to the root transaction, ie. in contexts.trace._metrics_summary?
  • We only add to the summaries when value: number and ignore metrics with value: string. It looks like this didn't come up in Python
  • The Python code used type:name@unit as the key for each metric summary. If the unit is not set, I guess we drop @metric from the end? The integration test I added has @undefined 😆
  • No idea how this will work yet post v8 in node where the spans are otel spans!
  • I haven't tested this against sentry.io yet but the envelopes look correct

@AbhiPrasad

This comment was marked as resolved.

Sign in to view
@AbhiPrasad

AbhiPrasad commented Jan 31, 2024

Copy link
Copy Markdown
Contributor

The Python code used type:name@unit as the key for each metric summary. If the unit is not set, I guess we drop @Metric from the end? The integration test I added has @undefined 😆

Yes I think we can drop @unit if the unit is not set.

No idea how this will work yet post v8 in node where the spans are otel spans!

Yeah this is TBD, but I think we might be able to hack span events to do this!

@AbhiPrasad

This comment was marked as resolved.

Sign in to view
cleptric
cleptric reviewed Jan 31, 2024
View reviewed changes
Comment threadpackages/core/src/metric-summary.ts Outdated
Comment threadpackages/core/src/metric-summary.ts Outdated
@timfish

This comment was marked as outdated.

Sign in to view
@timfish
timfish marked this pull request as ready for review February 1, 2024 00:13
@timfish
timfish requested a review from cleptricFebruary 1, 2024 00:47
@timfish

This comment was marked as resolved.

Sign in to view
AbhiPrasad
AbhiPrasad reviewed Feb 1, 2024
View reviewed changes
Comment threaddev-packages/node-integration-tests/suites/tracing/metric-summaries/scenario.js
Comment threadpackages/core/src/tracing/span.ts Outdated
Comment threadpackages/core/src/metrics/aggregator.ts Outdated
Comment threadpackages/core/src/metrics/metric-summary.ts Outdated
Comment threadpackages/core/src/metrics/aggregator.ts
@AbhiPrasad

AbhiPrasad commented Feb 5, 2024

Copy link
Copy Markdown
Contributor

image

Tested on Sentry and all seems good!

@AbhiPrasad
AbhiPrasad merged commit 94cdd8b into getsentry:developFeb 5, 2024
AbhiPrasad added a commit that referenced this pull request Feb 5, 2024
@AbhiPrasad
Revert "feat(core): Add metric summaries to spans (#10432)"
da8e9b6
This reverts commit 94cdd8b.
@AbhiPrasadAbhiPrasad mentioned this pull request Feb 5, 2024
Closed
AbhiPrasad added a commit that referenced this pull request Feb 5, 2024
@AbhiPrasad
Revert "feat(core): Add metric summaries to spans (#10432)" (#10495)
60a7d65
This reverts commit 94cdd8b.
I mistakenly merged this - we have to fix the metric summaries format.
Summaries are typed as `pub type MetricSummaryMapping =
Object<Array<MetricSummary>>;`
Which means that the `_metrics_summary` type needs to be
`_metrics_summary?: Record<string, Array<MetricSummary>>;`. This is
because we need to create separate entries if the tags have changed.
```
"c:processor.item_processed": [
{
"min": 1,
"max": 1,
"count": 3,
"sum": 3,
"tags": {"success": true}
},
{
"min": 1,
"max": 1,
"count": 2,
"sum": 2,
"tags": {"success": false}
}
],
```
@AbhiPrasad

AbhiPrasad commented Feb 5, 2024

Copy link
Copy Markdown
Contributor

Had to revert this - #10495

AbhiPrasad
AbhiPrasad reviewed Feb 5, 2024
View reviewed changes
Comment threaddev-packages/node-integration-tests/suites/tracing/metric-summaries/test.ts
const EXPECTED_TRANSACTION = {
transaction: 'Test Transaction',
_metrics_summary: {
'c:root-counter@none': {

@AbhiPrasadAbhiPrasadFeb 5, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this needs to be an array of metrics, based on what tags it has.

cleptric added a commit that referenced this pull request Feb 7, 2024
@cleptric
Revert "Revert "feat(core): Add metric summaries to spans (#10432)" (#…
bccf1ef
…10495)"
This reverts commit 60a7d65.
# Conflicts:
#	packages/types/src/event.ts
cleptric added a commit that referenced this pull request Feb 8, 2024
@cleptric
Revert "Revert "feat(core): Add metric summaries to spans (#10432)" (#…
7a6736a
…10495)"
This reverts commit 60a7d65.
# Conflicts:
#	packages/types/src/event.ts
cleptric added a commit that referenced this pull request Feb 8, 2024
@cleptric
Revert "Revert "feat(core): Add metric summaries to spans (#10432)" (#…
61e9a77
…10495)"
This reverts commit 60a7d65.
# Conflicts:
#	packages/types/src/event.ts
@timfish
timfish deleted the feat/metric-summaries branch February 19, 2024 15:47
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cleptriccleptricAwaiting requested review from cleptric
1 more reviewer
@AbhiPrasadAbhiPrasadAbhiPrasad approved these changes
Reviewers whose approvals may not affect merge requirements

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

@timfish@AbhiPrasad@cleptric