Initial commit for profcheck tool. #12
Conversation
|
|
||
| func checkStackTable(stackTable []*profiles.Stack, lenLocTable int) error { | ||
| if err := checkZeroVal(stackTable); err != nil { | ||
| return err |
There was a problem hiding this comment.
Is it necessary to treat this error differently than the others? Perhaps you could run run the for loop below regardless of what happens here, and prepend this error if it was found. It's valuable to see all errors in the table, even if I don't have the zero-entry for some reason.
There was a problem hiding this comment.
Yes, makes sense, I think it should just append and not bail out early. Basically it should behave like other checks. Will do.
| strIdxs := map[string]int{} | ||
| for idx, s := range strTable { | ||
| if origIdx, ok := strIdxs[s]; ok { | ||
| errs = errors.Join(errs, fmt.Errorf("duplicate string at index %d, orig index %d: %s", idx, origIdx, s)) |
There was a problem hiding this comment.
Is this part of the spec? I cannot find it.
There was a problem hiding this comment.
We discussed in a recent SIG meeting or two that we want to add a couple of things at "should do, unless you know what you are doing":
- All dictionary tables should generally have no duplicate entries.
- All dictionary tables should generally have no orphaned entries (entries that are not referenced).
But we also said that these are not mandates. I need to figure out what's the best way to handle this in the conformance checker. I thought of switching from the error model to some kind of logging model with severity etc, but this will complicate things. Currently I'm thinking of simply having a flag for optional checks, off by default, and guard the two mentioned checks above with it.
And here specifically it's a bug that I need to fix - in that this should be an optional check.
| return errors.New("resource profiles are empty") | ||
| } | ||
| for _, resourceProfiles := range data.ResourceProfiles { | ||
| // TODO: Check attributes? |
There was a problem hiding this comment.
Maybe add a check to verify that a ResourceProfile holds at least one ScopeProfile.
| for _, resourceProfiles := range data.ResourceProfiles { | ||
| // TODO: Check attributes? | ||
| for _, scopeProfiles := range resourceProfiles.ScopeProfiles { | ||
| // TODO: Check attributes? |
There was a problem hiding this comment.
Maybe add a check to verify that a ScopeProfile holds at least one Profiles.
| "google.golang.org/protobuf/proto" | ||
| ) | ||
|
|
||
| func CheckConformance(data *profiles.ProfilesData) error { |
There was a problem hiding this comment.
If ebpf-profiler should make use of this, some additional work is needed to transform go.opentelemetry.io/proto/otlp/profiles/v1development to go.opentelemetry.io/collector/pdata/pprofile.
| return errors.New("resource profiles are empty") | ||
| } | ||
| for _, resourceProfiles := range data.ResourceProfiles { | ||
| // TODO: Check attributes? |
There was a problem hiding this comment.
As Attributes in ResourceProfile are not part of the ProfilesDictionary, I think we don't need to check them.
| for _, resourceProfiles := range data.ResourceProfiles { | ||
| // TODO: Check attributes? | ||
| for _, scopeProfiles := range resourceProfiles.ScopeProfiles { | ||
| // TODO: Check attributes? |
There was a problem hiding this comment.
As Attributes in ScopeProfiles are not part of the ProfilesDictionary, I think we don't need to check them.
| for i, strIdx := range prof.CommentStrindices { | ||
| if err := checkIndex(len(dict.StringTable), strIdx); err != nil { | ||
| errs = errors.Join(errs, prefixErrorf(err, "comment_strindices[%d]", i)) | ||
| } | ||
| } |
There was a problem hiding this comment.
As the idea is to drop this field in the proto, we maybe can already drop the check here.
| for i, strIdx := range prof.CommentStrindices { | |
| if err := checkIndex(len(dict.StringTable), strIdx); err != nil { | |
| errs = errors.Join(errs, prefixErrorf(err, "comment_strindices[%d]", i)) | |
| } | |
| } |
| errs = errors.Join(errs, fmt.Errorf("timestamps_unix_nano[%d]=%d is outside profile time range [%d, %d]", i, tsUnixNano, startUnixNano, endUnixNano)) | ||
| } | ||
| } | ||
| // TODO: Add a check for the value vs timestamp shapes. |
There was a problem hiding this comment.
We might also add a check for the number of values/timestamps.
// A Sample MUST have have at least one values or timestamps_unix_nano entry. If
// both fields are populated, they MUST contain the same number of elements
3 participants
An initial version of the OpenTelemetry Profiles signal format conformance checker.
There are a few things remaining: