Closed tsloughter closed 3 years ago
Yeah, module attributes are just constants for that module. I'm actually not really sure what went into using macros to define atoms in the Erlang side and why they're capitalized, but just using atoms directly and having them be lower case would remove the weirdness of the Elixir usage. What was the reason for going that route?
Since other BEAM based languages may not use erlang macro, and lowercase atom seems more "idiomatic" in Erlang... it would be better to define list of lowercase atoms with type definition. erlang library may use macro in it though..
The reason it is :Error
is because that is the Otel value. The user isn't supposed to see it so it not being idiomatic wasn't supposed to matter. The user should see only a constant, guarding against typos or if we ever need to change terms behind the scenes.
@tsloughter Thanks for the comment. Here are my questions and suggestions:
The reason it is :Error is because that is the Otel value
What is "the Otel value"?
I found that those values (e.g. SERVER
for SpanKind and Error
for Status) are not even the exact values will be used in messages, since 1) OpenTelemetry supports both gRPC (protobuf - so integer value will be used) and HTTP (via JSON mapping, which uses the protobuf label name), so no single "value" there but this library's internal representation and 2) this library actually translates the internal values (OTEL_STATUS_ERROR
) to OTLP value (ref) (e.g. STATUS_CODE_ERROR
) as the spec says.
So why not using lowercase atoms (e.g. error
) which are idiomatic? Similar to that most HTTP libraries use lowercase atoms for HTTP request (e.g. take post
, not "POST"
, and use the corresponding HTTP method).
The user isn't supposed to see it so it not being idiomatic wasn't supposed to matter
I think it should be rather "users care the interface to this library while not caring what value this library will use in messages behind the scene."
As a dev, I prefer to have consistent, erlang/elixir idiomatic way to use libraries, whenever possible.
For flexibility and extensibility, this library may accept both atom (e.g. server
) for known values (which will be translated) or strings (e.g. "SERVER"
) to support values added in the future, or to use the value as-is.
@chulkilee by "otel value" I simply meant the quoted term used in https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/trace/api.md#set-status and for Kind
.
is it common in Erlang world?
No, in Erlang the macros are used. Those functions were only added as a way for the Elixir code to access those macro values.
Exposing functions is the only way Elixir offers to expose constant values, I'd have used another way if it was available.
If it is this big of a deal then we can switch to lowercase (tho I find that odd too since no matter what in Elixir you have to have the :
prefix :) to be idiomatic in Elixir.
I think it should be rather "users care the interface to this library while not caring what value this library will use in messages behind the scene."
Na, it is more than that, as a user I want to be told at compile time if this is wrong. With macros in Erlang this is done if I mistype a Kind macro I'm told, not so if I put the atom sever
instead of server
-- it hopefully gets caught by dialyzer but not good to bet on.
But again, I'm fine with a patch to lowercase everything if that is what Elixir users expect instead of functions for constants.
Thanks @tsloughter
For the value:
It seems like OpenTelemetry API specification (e.g. Tracing API) defines possible values without specific representation, and OpenTelemetry Protocol specification defines own ProtoBuf schema and JSON mapping from the schema for representation of the values in gRPC/HTTP protocoal.
Also Tracing API says:
While languages and platforms have different ways of representing data, this section defines some generic requirements for this API.
Therefore actual representation inside library doesn't matter. For example, python and java libraries use their canonical way to represent the status code values - not using the "Otel value" (e.g. Error
).
So no need to use the "Otel value" in the API spec as-is. My suggestion is to use lowercase atom for all those values, since lowercase is more widely used in Erlang. Hopefully before hitting 1.0 😄
For how to get the value:
Na, it is more than that, as a user I want to be told at compile time if this is wrong.
Oh I also want that :wink: but I'm against requiring calling functions to get the enum-like atom value defined by library. There is no need to "hide" them unless the library wants to change the representation freely without users' changes. Also note that I can even dynamically call functions (e.g. apply
) so there is no 100% guranteed compile-time check if a user is not using it as intended..
But yeah I really hope I have enum-like value for constant with compile-time check... but that's not just available in Elixir out-of-box. Using macro is one option, but it has same limitation with calling functions 😢
Right, it isn't a requirement, I've just found it simpler to default to what the API says to get around discussion/disagreement about naming. Obviously a number of things differ, we don't use EndSpan
but instead end_span
. So changing to lowercase is fine.
But again, the values aren't supposed to be used, so lowercase being how Erlang does atoms doesn't matter :) No one in Erlang would be writing 'Error'
.
I think #175 resolves this issue.
I noticed in https://github.com/opentelemetry-beam/opentelemetry_phoenix/pull/16 that since Elixir (as far as I know, and seems confirmed by the code in the PR) can't use macros from Erlang headers the user code has to use explicit atoms like
:SERVER
and:Error
for Kind and Status.I'm not sure what the equivalent is in Elixir (I know there are module attributes with
@name
but those are just within the module right?) but will find out and unless someone gets to it before me.