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

Allow updating enum values when referencing attribute #192

Open
lmolkova opened this issue Jul 28, 2023 · 9 comments
Open

Allow updating enum values when referencing attribute #192

lmolkova opened this issue Jul 28, 2023 · 9 comments
Labels
enhancement New feature or request

Comments

@lmolkova
Copy link
Contributor

Semantic conventions that reference a common attribute with an enum type, should be able to extend/overwrite the list of possible values.

E.g.

- id: http.request.method
  type:
    allow_custom_values: true
    members:
      - id: delete
        value: "DELETE"
        brief: 'DELETE method.'
      - id: get
        value: "GET"
        brief: 'GET method.'
...

- ref: http.request.method
  type:
    allow_custom_values: true
    members:
      - id: purge
        value: "PURGE"
        brief: 'non-standard PURGE method.'

currently redeclaring a type is not supported.

While additional values could be documented in the note, md generation adds another note with possible values according to enum.

Related: open-telemetry/semantic-conventions#205

@joaopgrassi
Copy link
Member

There's a need for this in open-telemetry/semantic-conventions#258. There's several attributes that are used in different metrics like hw.status. From @bertysentry :

The attributes do have the same meaning across all device types. However, we may want to add an additional values that are specific a type of device. Example: we may want to specify the value charging for the state attribute in the hw.status metric, that is applicable to batteries only (charging).

If these are defined as enums, when -ref them in metrics we will need to add more values to it.

@Oberon00
Copy link
Member

Oberon00 commented Aug 23, 2023

This seems like a bit of a hack to me and maybe a mis-use of "ref" to compensate for a lacking feature. Would it be better to first decouple the enum definition from the attribute definition and define a top-level "types" section alongside "groups"?

@lmolkova
Copy link
Contributor Author

Would it be better to first decouple the enum definition from the attribute definition and define a top-level "types" section alongside "groups"?

would it mean that the same attribute would have different type for different conventions? Or that we'd need to introduce type inheritance?

@Oberon00
Copy link
Member

Oberon00 commented Aug 24, 2023

Both, I think 😃
(It's the same with the ref: suggestion though, at least I would say that the type becomes a different one if you add values, and that the ref acts as inheritance mechanism)

@lmolkova
Copy link
Contributor Author

lmolkova commented Aug 24, 2023

BTW I believe enums being enums in code generation is a back-compat pain - when value is removed it breaks every lib that used it.

I think they should be generated as maps instead of enums and then this issue can be resolved without type inheritance or type hell in strongly typed languages.

for example, we have expandable enums (backed by a map) implementation in Azure SDKs, Here's an example in Java

public final class CloudEventDataFormat extends ExpandableStringEnum<CloudEventDataFormat> {
    public static final CloudEventDataFormat BYTES = fromString("BYTES", CloudEventDataFormat.class);
    public static final CloudEventDataFormat JSON = fromString("JSON", CloudEventDataFormat.class);

    public static CloudEventDataFormat fromString(String name) {
        return super.fromString(name, CloudEventDataFormat.class);
    }
}

essentially it feels like an enum, but can create new instances of itself for unregistered strings.

@lmolkova
Copy link
Contributor Author

lmolkova commented Aug 24, 2023

(or maybe we can introduce new map type so we don't change behavior for existing enums at once)

@Oberon00
Copy link
Member

Oberon00 commented Aug 25, 2023

when value is removed it breaks every lib that used it

That will be the case no matter how you implement code generation. You can at most make it a runtime error instead of a compile time error which would be bad anyways. Removing values from enums is not covered by our stability guarantees I think, but there is no sensible way to implement instrumentations so defensively that they would survive removal of an enum value they actually use without code changes.

@lmolkova
Copy link
Contributor Author

lmolkova commented Aug 25, 2023

Watch me implement it in a sensible and back-compatible way: SemConv.HttpMethods.fromString("PURGE")

@lmolkova
Copy link
Contributor Author

lmolkova commented May 1, 2024

Another example is *.system

E.g. db.system has ~50 values.
When we define system-specific semconv, we reference db.system and (if we get rid of not-full tables), we end up listing all 50 in each extension: open-telemetry/semantic-conventions#973

I'd like to be able to do something like

- id: db
   ...
   attributes:
    - id: db.system
      type: 
        members: 
        - id: cassandra
        - id: mongodb
        ...

- id: mongodb
   attributes:
    - ref: db.system
      type: 
        # some flag indicating that I don't want to extend everything?
        members: 
        - ref: mongodb

in this case I want only once value to be supported and not inherit all 50 values.

@lmolkova lmolkova changed the title Allow extending enum values when referencing attribute Allow updating enum values when referencing attribute May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Improve YAML Schema
Development

No branches or pull requests

3 participants