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.

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

Propose OpenTelemetry Profiling Vision #212

Merged
merged 17 commits into from
Sep 22, 2022

Conversation

Rperry2174
Copy link
Contributor

@Rperry2174 Rperry2174 commented Aug 30, 2022

This change proposes high-level items that define our long-term vision for
Profiling support in OpenTelemetry project.

A group of open source maintainers, vendors, end-users, and developers excited about profiling / otel have been meeting for ~8 weeks now and this document was created collectively in order to share with the broader community for feedback. We've had ~15 people contribute directly to the document and over 60 people who have attended our meetings over the past couple of months.

This idea of "Adding profiling as a support event type" has also been discussed at length in this issue created in October 2020.

If this proposal is accepted/approved then we will proceed with filling out this project tracking issue and following the other procedures outlined in the project management instructions.

Comments, ideas, feedback, etc. are all very welcome and highly appreciated!

@Rperry2174 Rperry2174 requested a review from a team August 30, 2022 05:57
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 30, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Rperry2174 / name: Ryan Perry (96cf8da)

text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, thank you.

Approving, this is aligned with my vision as well.

text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
Co-authored-by: Tigran Najaryan <4194920+tigrannajaryan@users.noreply.github.com>
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
Rperry2174 and others added 3 commits September 6, 2022 09:15
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
Co-authored-by: Reiley Yang <reyang@microsoft.com>
@halcyondude
Copy link

+1 NB

Copy link

@SeanHeelan SeanHeelan left a comment

Choose a reason for hiding this comment

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

lgtm, just a couple of minor wording fixes from the performance section I added in the original doc.

text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
@carlosalberto
Copy link
Contributor

Overall LGTM - I'm a little curious about the initial implementation and its target languages (it seems that it has been informally established the effort will start with Go and Java profiling?)

@yurishkuro
Copy link
Member

I am supportive of this vision, but there seems to be a glaring omission in the document - the terms "profile" and "profiling" are never defined. Since this is essentially proposing a new type of telemetry signal to be included in the project, can you provide a clear definition?

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Just some minor nits, otherwise LGTM!

text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Outdated Show resolved Hide resolved
@halcyondude
Copy link

halcyondude commented Sep 9, 2022

This captures the vision and spirit of the discussions over the past few months, and is a good position from which to start this collaborative effort. I support!

+1

Copy link

@kittylyst kittylyst left a comment

Choose a reason for hiding this comment

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

Decent statement of high-level intent.

text/profiles/0212-profiling-vision.md Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Show resolved Hide resolved
text/profiles/0212-profiling-vision.md Show resolved Hide resolved
@chadbrewbaker
Copy link

If I had to make it more concrete - I might adopt the Rust targets for OS/libc/CPU as a first approximation - especially for embedded CPUs.

On a geospatial level - lat/long would be useful data for network flow modeling. Also network triangulation like pings to three servers - bandwidth might be harder to measure.

@Rperry2174
Copy link
Contributor Author

I believe all comments have been addressed let me know if anything else needs to be before merging!

@tigrannajaryan
Copy link
Member

This vision now has a large number of approvals from existing OpenTelemetry members, from OpenTelemetry Technical Committee and Governance Committee and from the new Profiling Workgroup members.

I believe this shows a clear alignment on the vision from all participants.

Members of @open-telemetry/technical-committee if you haven't had a chance to review this vision, please do.

I will merge the PR and will consider this current vision accepted by OpenTelemetry Profiling if I don't see any further comments.

@tigrannajaryan
Copy link
Member

No new comments. Lots of approvals. Merging.

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.