Skip to content

Conversation

@fandreuz
Copy link
Contributor

@fandreuz fandreuz commented Jun 4, 2025

Fixes #4220, and reintroduces the changes from #4212.

In addition to #4212, this has two more commits:

  • 8814dcf: graceful error handling for malformed OTLP data
  • b8b03cd: new tests with malformed data

@fandreuz fandreuz requested review from a team and korniltsev as code owners June 4, 2025 01:22
@fandreuz fandreuz changed the title Bump OTLP to v1.7.0 and handle gracefully malformed OTLP profiles Bump OTLP to v1.7.0 and handle malformed OTLP profiles gracefully Jun 4, 2025
@fandreuz fandreuz changed the title Bump OTLP to v1.7.0 and handle malformed OTLP profiles gracefully Bump OTLP to v1.7.0 and handle malformed OTLP profiles Jun 4, 2025
@kolesnikovae kolesnikovae self-assigned this Jun 4, 2025
@kolesnikovae
Copy link
Collaborator

Thank you @fandreuz for adding this!

It looks very good overall, and I'm going to review it carefully tomorrow. @korniltsev, if you get a chance, it would be great if you could take a look as well

Copy link
Collaborator

@kolesnikovae kolesnikovae left a comment

Choose a reason for hiding this comment

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

Thank you for all the effort, @fandreuz – it looks great! We'll dogfood some real OTLP profiles over an extended period and plan to merge it next week.

@fandreuz
Copy link
Contributor Author

fandreuz commented Jun 10, 2025

Hi @kolesnikovae, I noticed Pyroscope enforces the presence of a mapping for each location. According to the spec, that field is optional. Profiles not carrying it should probably be accepted without errors.

I'll fix the merge conflicts later today

Copy link
Contributor

@simonswine simonswine left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! I have been running this against the latest otel-ebpf-profiler and works like a charm.

LGTM

@simonswine simonswine merged commit 5746aa8 into grafana:main Jun 13, 2025
25 checks passed
@simonswine
Copy link
Contributor

Hi @kolesnikovae, I noticed Pyroscope enforces the presence of a mapping for each location. According to the spec, that field is optional. Profiles not carrying it should probably be accepted without errors.

@fandreuz This in fact will change with the next version, as very recently some PR marked this as no longer optional. But feel free to follow up with another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sanitize OTLP profiles

3 participants