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

String representation of component.StabilityLevel and configtelemetry.Level is inconsistent with other types #6490

Closed
dmitryax opened this issue Nov 7, 2022 · 10 comments
Assignees

Comments

@dmitryax
Copy link
Member

dmitryax commented Nov 7, 2022

String methods of all types in pdata module along with service.State return capitalized strings, for example:

  • pmetric.AggregationTemporality.String(): "Unspecified", "Delta" or "Cumulative"
  • service.State.String(): "Starting", "Running", "Closing", etc.

But component.StabilityLevel and configtelemetry.Level return lowercase strings:

  • component.StabilityLevel.String(): "unmaintained", "deprecated", "in development", etc.
  • configtelemetry.Level.String(): "none", "basic", "normal" or"detailed"

Should we align the to the same Capitalized format?

Or there is still a chance to make all other strings lowercase if we think that's a better option.

cc @bogdandrutu @codeboten @Aneurysm9

@bogdandrutu
Copy link
Member

I think I like the "Capitalized" version more.

@Aneurysm9
Copy link
Member

I also prefer capitalization. It aligns with my expectation that Capitalized Terms are well-defined somewhere in the domain and may carry denotations beyond what might otherwise be considered as their plain meaning.

@bogdandrutu
Copy link
Member

bogdandrutu commented Nov 8, 2022

@dmitryax for configs I think we should allow any capitalization? I think it is fine if in the config file (like configtelemetry.Level) we allow users to write "none", "NONE" or "None".

**Update:" this may be a separate issue to discuss, but is related at least with configtelemetry.Level.

@dmitryax
Copy link
Member Author

dmitryax commented Nov 8, 2022

@dmitryax for configs I think we should allow any capitalization? I think it is fine if in the config file (like configtelemetry.Level) we allow users to write "none", "NONE" or "None".

Yes, that's how it's currently done in configtelemetry.Level, input of any case is supported

@codeboten
Copy link
Contributor

Capitalized is fine with me.

@dmitryax dmitryax self-assigned this Nov 8, 2022
@dmitryax
Copy link
Member Author

dmitryax commented Nov 8, 2022

Sounds good! I'll submit a PR to capitalize component.StabilityLevel and configtelemetry.Level strings.

@bogdandrutu
Copy link
Member

For CompressionType there are some things:

  • Capitalization does not make that much sense
  • Maybe rename to Compression, but anyway we also need to fix the enum value names.

@codeboten
Copy link
Contributor

codeboten commented May 8, 2023

@dmitryax is this completed? Is there still something left to do here?

@atoulme
Copy link
Contributor

atoulme commented Dec 19, 2023

It looks like the original issue is completed. Bogdan mentions we could look at renaming CompressionType Compression additionally.

@TylerHelmuth
Copy link
Member

I've captured the configcompression piece in its own issue. Closing this as completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

6 participants