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

[semantic-conventions] provide constants for literal string values to support use as match conditions #685

Closed
polivbr opened this issue Dec 29, 2021 · 7 comments
Labels
A-common Area:common issues that not related to specific pillar

Comments

@polivbr
Copy link

polivbr commented Dec 29, 2021

Currently, the Key values in the semantic conventions crate cannot be used as conditions inside of a match expression because the inner Cow does not derive PartialEq and Eq.

While it would be nice to be able to use the Keys directly as match conditions, I appreciate that moving away from a Cow based implementation isn't feasible.

What I think would be a workable alternative would be to have constants not only for the Keys, but for the the literal strings used to construct the Keys. With those, the string constants could be used as match conditions even if the Keys cannot.

@jtescher
Copy link
Member

Could consider having both constants, you should be able to use Key::as_str as a workaround to check equality as well.

@polivbr
Copy link
Author

polivbr commented Dec 30, 2021

Right now I'm using Key::as_str with a long chain of if/else statements, as it can't be used in a match statement.

@djc
Copy link
Contributor

djc commented Dec 30, 2021

I'm confused by the issue description: first, there is no Key defined in -semantic-conventions, second, Key itself does have derived PartialEq and Eq implementations. If you mean that Key should have an impl PartialEq<str> for Key, that's trivial, but I don't think it solves your issue:

impl PartialEq<str> for Key {
    fn eq(&self, other: &str) -> bool {
        self.0.as_ref() == other
    }
}

fn foo() {
    match Key::new("foo") {
        "foo" => true,
        _ => false,
    };
}

still errors with

error[E0308]: mismatched types
   --> opentelemetry/src/common.rs:109:9
    |
108 |     match Key::new("foo") {
    |           --------------- this expression has type `Key`
109 |         "foo" => true,
    |         ^^^^^ expected struct `Key`, found `&str`

Still, I don't understand your statement that Key::as_str can't be used in match statement, this seems fine:

fn foo() {
    match Key::new("foo").as_str() {
        "foo" => true,
        _ => false,
    };
}

@polivbr
Copy link
Author

polivbr commented Dec 30, 2021

not sure what you mean by there being no Key defined. You even refer to Key in your own examples.

from resource.rs:

use opentelemetry::Key;
...
pub const CLOUD_PROVIDER: Key = Key::from_static_str("cloud.provider");
...

I'm not looking to match the Key against a list of string values, I'm looking to match a string against a list of Keys:

match Key::new(string_from_external_source) {
   resource::SERVICE_VERSION => ...,
   resource::SERVICE_NAME => ...
}

It doesn't matter if Key implements Eq and PartialEq. The internal Cow would also need to derive them, which it doesn't. Here's a related issue from the rust-postgres repo:

sfackler/rust-postgres#756

@hdost
Copy link
Contributor

hdost commented Feb 21, 2024

@KallDrexx weren't you working on stuff related to this?

@hdost hdost added the A-common Area:common issues that not related to specific pillar label Feb 21, 2024
@djc
Copy link
Contributor

djc commented Feb 21, 2024

Isn't this #1334?

@hdost
Copy link
Contributor

hdost commented Feb 21, 2024

Yes it seems so 👍

@hdost hdost closed this as completed Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-common Area:common issues that not related to specific pillar
Projects
None yet
Development

No branches or pull requests

4 participants