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.

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

Add doc_url field to match its recent addition in pprof proto. #588

Closed
wants to merge 2 commits into from
Closed

Add doc_url field to match its recent addition in pprof proto. #588

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c8e616
Add doc_url field to match its recent addition in pprof proto.
aalexand Sep 12, 2024
854e118
Update opentelemetry/proto/profiles/v1experimental/pprofextended.proto
aalexand Sep 12, 2024
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
7 changes: 7 additions & 0 deletions opentelemetry/proto/profiles/v1experimental/pprofextended.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ message Profile {
// Index into the string table of the type of the preferred sample
// value. If unset, clients should default to the last sample value.
int64 default_sample_type = 14;
// Documentation link for this profile. The URL must be absolute,
// e.g., http://pprof.example.com/cpu-profile.html
//
// The URL may be missing if the profile was generated by older code or code
// that did not bother to supply a link.
int64 doc_url = 19; // Index into string table.
Copy link
Contributor

Choose a reason for hiding this comment

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

With ScopeProfiles.schema_url the URL that describes the provided profile exists already. Where do you see the differences between the two, so that this additional field is required?

Copy link
Author

Choose a reason for hiding this comment

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

schema_url points to https://opentelemetry.io/docs/specs/otel/schemas/#schema-url file which is not user docs. The doc_url here is for user-level documentation. I would also expect that the schema URL would be the same for many profiles as it corresponds to the version of the data schema or something like that rather than the nature of the data (CPU profile vs heap profile vs contention profile).

Copy link
Member

@christos68k christos68k Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Since this should be an optional field, what do you think about using an attribute?

On a similar note, we had a discussion in the last SIG about the boolean fields in Mapping and the consensus was to drop them from the proto and turn them into attributes.

Copy link
Author

Choose a reason for hiding this comment

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

I think the doc_url can be an attribute. I'll change this.

For the has_* fields I'm less sure. I added an agenda topic to the SIG notes doc with some thoughts on this.


}

// Represents a mapping between Attribute Keys and Units.
Expand Down