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

[configcompression] Standardize enums #9388

Closed
TylerHelmuth opened this issue Jan 24, 2024 · 14 comments · Fixed by #9517
Closed

[configcompression] Standardize enums #9388

TylerHelmuth opened this issue Jan 24, 2024 · 14 comments · Fixed by #9517

Comments

@TylerHelmuth
Copy link
Member

TylerHelmuth commented Jan 24, 2024

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.

Originally posted by @bogdandrutu in #6490 (comment)

The constants in configcompression don't fit the same pattern as component.StabilityLevel and configtelemetry.Level: do the CompressionType consts also need capitalized or are they exempt? Do any of the public members need name changes?

@TylerHelmuth
Copy link
Member Author

There is a lot of good history for this package here: #4651. This is actually the only PR ever made for configcompression.

@TylerHelmuth
Copy link
Member Author

My opinion is that:

  1. The values do no need "Capitalized"
  2. The const names are proper
  3. CompressionType is a good name.

which is the same as "this package doesn't need any public name changes".

@dmitryax
Copy link
Member

The values do no need "Capitalized"

I agree

The const names are proper

It's inconsistent with other types constants that start with the type. Ideally should be CompressionTypeGzip instead of Gzip but given that the package is configconmpression maybe unnecessary.

CompressionType is a good name

If we leave 2 as is, I'd say we should change CompressionType to Compression.

We can also have configconmpression.Type and configconmpression.TypeGzip. That would be consistent with all other constants in the project.

@TylerHelmuth
Copy link
Member Author

It's inconsistent with other types constants that start with the type. Ideally should be CompressionTypeGzip instead of Gzip but given that the package is configconmpression maybe unnecessary.

I believe this discussion came up in the original PR and Compression was dropped bc of the package name.

We can also have configconmpression.Type and configconmpression.TypeGzip. That would be consistent with all other constants in the project.

I see that this would align with component.Type but what else?

@dmitryax
Copy link
Member

I see that this would align with component.Type but what else?

What do you mean what else? If we have configconmpression.Type as type and configconmpression.TypeGzip as consts it'll be consistent with all other use cases in Collector, e.g. component.DataType and component.DataTypeTraces

@TylerHelmuth
Copy link
Member Author

Ah I went looking specifically for other packages that used just Type. If Type is the word we like to use when defining enums then I agree on that name change. That would align it with pdata and component.

@TylerHelmuth
Copy link
Member Author

Current situations we should finalize for enum naming:

  1. A package contains the "object" in the package name and contains enums for that "object"
    • such as component.Type and the proposed configcompression.Type
  2. A package contains enums but they are not directly related to the package name.

In situation 1 should we standardize on the enum being named Type? If we do, does that mean pmetric.MetricType is out of compliance with the standard?

In situation 2, should we standardize on including or excluding Type in the enum name? Currently pcommon.ValueType sets a precedence to include Type.

@TylerHelmuth
Copy link
Member Author

For situation 1 my opinion is that using only Type is really nice, but the fact that pmetric.MetricType isn't doing that makes me think we either don't need to strictly enforce this situation or we should use the precedence set by `pdata.

For situation 2, my opinion is that we include Type in the name. Not only has pdata set that precedence, but I think that Type in the name quickly conveys that this is used for enums.

@mx-psi
Copy link
Member

mx-psi commented Jan 31, 2024

I think to move forward we should add a section about enumerations in CONTRIBUTING.md. The questions that we should solve there are:

  1. Should an enumeration be a type alias or a struct? (Personally I lean towards type alias because that's how it's done everywhere in Go, even if it has downsides)
  2. If it's a type alias, what should the underlying type of the enumeration be? (e.g. should we always use an int to reduce memory usage, is string okay?)
  3. When should enumeration values be prefixed?
  4. When should enumeration types have the Type suffix?

@TylerHelmuth would you be up for filing such a PR? We can continue the discussion there much more easily and hopefully solve this question for all modules at once :)

@TylerHelmuth
Copy link
Member Author

@mx-psi ya I can open that PR

@dmitryax
Copy link
Member

dmitryax commented Jan 31, 2024

For situation 1 my opinion is that using only Type is really nice, but the fact that pmetric.MetricType isn't doing that makes me think we either don't need to strictly enforce this situation or we should use the precedence set by `pdata.

I think we should use Type only if the package name represents an entity of the type. It's not the case for pmetric. pmetric.Metric is a struct for one metric while pmetric is a package for pipeline metrics data

@dmitryax
Copy link
Member

I think configcompression can be used as something that Type represents. If you think it doesn't, I'm good with configcompression.CompressionType as well as long as all enums are prefixed with it.

@dmitryax
Copy link
Member

dmitryax commented Jan 31, 2024

Should an enumeration be a type alias or a struct? (Personally I lean towards type alias because that's how it's done everywhere in Go, even if it has downsides)

I think we've always been using type definitions. It provides better protection from misuse. So I think we should stick with them

bogdandrutu pushed a commit that referenced this issue Feb 1, 2024
**Description:** 
Adds expectations for how the repository handle enumerations

**Link to tracking Issue:** <Issue number if applicable>
Relates to
#9388
mx-psi added a commit that referenced this issue Feb 5, 2024
**Description:**
If we choose to go with the rename.

**Link to tracking Issue:** <Issue number if applicable>
Related to
#9388

---------

Co-authored-by: Pablo Baeyens <pablo.baeyens@datadoghq.com>
@mx-psi
Copy link
Member

mx-psi commented Feb 7, 2024

I think this was solved by #9416, right?

mx-psi added a commit that referenced this issue Feb 8, 2024
…9517)

**Description:**

Removes deprecated types, constants and methods.

**Link to tracking Issue:** Fixes #9388
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants