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

[pdata] Add documentation defining naming conventions #6255

Merged
merged 1 commit into from
Oct 18, 2022

Conversation

dmitryax
Copy link
Member

@dmitryax dmitryax commented Oct 7, 2022

The documentation is intended to summarize all the naming rules followed in pdata module

@dmitryax dmitryax added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Oct 7, 2022
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 92.48% // Head: 92.48% // No change to project coverage 👍

Coverage data is based on head (aeccca5) compared to base (806ba4c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6255   +/-   ##
=======================================
  Coverage   92.48%   92.48%           
=======================================
  Files         218      218           
  Lines       13124    13124           
=======================================
  Hits        12138    12138           
  Misses        769      769           
  Partials      217      217           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dmitryax dmitryax force-pushed the pdata-deadme branch 2 times, most recently from 86413a3 to 9e948b9 Compare October 7, 2022 21:58
@dmitryax dmitryax marked this pull request as ready for review October 7, 2022 21:58
@dmitryax dmitryax requested review from a team and mx-psi October 7, 2022 21:58
@dmitryax dmitryax changed the title [WIP] [pdata] Add documentation defining naming conventions [pdata] Add documentation defining naming conventions Oct 7, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Oct 7, 2022

@open-telemetry/collector-approvers PTAL

pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Show resolved Hide resolved
@dmitryax dmitryax force-pushed the pdata-deadme branch 3 times, most recently from bb13b8d to e3f3eb1 Compare October 10, 2022 19:37
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.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.

I think it is useful to document the design principles of pdata API, but I think it would be beneficial to remove from this document the parts which are already codified in the pdata generator to avoid the need to maintain the document if the generator is changed in any way.

pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
Comment on lines +28 to +47
- `plog.Logs` based on `LogsData` protobuf message.
- `pmetric.Metrics` based on `MetricsData` protobuf message.
- `ptrace.Traces` based on `TracesData` protobuf message.
- `pcommon.Slice` based on `ArrayValue` protobuf message.
- `pcommon.Map` based on `KeyValueList` protobuf message.
- `pcommon.Value` based on `AnyValue` protobuf message.
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to delete all these lines and only keep one exception example, to reduce the amount of maintenance this document requires if the source code is refactored? Let the source code be the source of truth. If you want the fact of the exception to be documented it would be better to add a comment in the source code near the name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. Removed

pdata/README.md Outdated Show resolved Hide resolved
pdata/README.md Outdated Show resolved Hide resolved
Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

Please document all exceptions, see comment
#6283 (comment)

@dmitryax
Copy link
Member Author

dmitryax commented Oct 11, 2022

Please document all exceptions, see comment #6283 (comment)

We had the struct name exceptions listed in this document initially.

@tigrannajaryan suggested to not list them all, but use the code as source of truth #6255 (comment).

If we want to document field name exceptions, I believe we should do the same for the structs. So the question is where we document the exceptions: source code comments or this Readme file.

Using the readme adds maintenance burden as Tigran pointed out. But I don't like putting them into the comments because the comments are intended for API users who probably don't care why particular type or field is called one way or another. So I would rather list them here, and I don't think it'll be a lot of them going forward to bother us with keeping it in sync.

Co-authored-by: Pablo Baeyens <pbaeyens31+github@gmail.com>
Co-authored-by: Anthony Mirabella <a9@aneurysm9.com>
@dmitryax
Copy link
Member Author

@bogdandrutu, I mentioned all the naming exceptions in this doc

@bogdandrutu
Copy link
Member

Not perfect, but a great start :) Thanks @dmitryax

@bogdandrutu bogdandrutu merged commit e394ae0 into open-telemetry:main Oct 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants