opentelemetry-erlang-contrib
                                
                                
                                
                                    opentelemetry-erlang-contrib copied to clipboard
                            
                            
                            
                        Add ecto adapter attribute
I couldn't find any discussion about how to include the required attribute db.system.
I think currently there isn't a good way to infer that information, maybe ecto adapters can send to telemetry that information through metadata. For now, if we simple pass the ecto adapter as attribute, will be possible to infer and map that info.
Elixir.Ecto.Adapters.Postgres -> postgresql
Elixir.Ecto.Adapters.MyXQL -> mysql
Elixir.Ecto.Adapters.Tds -> mssql
This PR do the following:
- 
Change some attributes key to follow semantic conventions.
 - 
Add the attribute
ecto.db.adapter - 
Add the attribute
db.system 
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: jhonndabi / name: Jonadabe Nascimento (2f77ab9072d36900e98357e10cab6f4c96b85250, f724c9b1aab6a87e6ae8e51134fdf6639aa93208, b638a1d790bbb8ad53ae000402cb4cd9d9c08831, 725a891035c4babe493fe0702bdcdc5036e510c7, f9817ba34cd9eec232b06f48f5f50daa82222c16, 40cf83ec96cb8527c7e885f12eedf8d001ce5235, e076e65d9db1579a6c88c3bc2e992effd8c26f70, f8ff0bbb4b93d7e71bac8336406c9440e03f9737)
 
Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.
I agree you can create a function to map the adapter name to the db.system name and add db.system attribute to this.
Can you open an issue on ecto to request it be added as an attribute in the meta?
Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.
I think this is supposed to be at the driver level which would be postgrex for the postgres adapter.
Could submit the ecto specific adapter attribute to https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/trace/semantic_conventions/database.md#connection-level-attributes-for-specific-technologies -- as far as I can tell it is the same as like a JDBC driver class name.
I think this is supposed to be at the driver level which would be postgrex for the postgres adapter.
Driver is a required option to sql adapter.
https://github.com/elixir-ecto/ecto_sql/blob/6935c289f68e1404ef0a6ec652d8fb1f89a82b96/lib/ecto/adapters/sql.ex#L88 https://github.com/elixir-ecto/ecto_sql/blob/6935c289f68e1404ef0a6ec652d8fb1f89a82b96/lib/ecto/adapters/sql.ex#L735
What do you think to ask them in the issue I will open to include driver and dbms_identifier in the telemetry metadata?
cc @bryannaegele @tsloughter
@jhonndabi sorry I missed this, that sounds like a good plan to me.
Also,j can you change the attribute to ecto.db.adapter.
@jhonndabi would you be ok adding db.system with just the mapping you mention in your first comment? Since that information is available, I don't see why we don't just do the mapping.
@tsloughter Ok, Sorry for the delay, I didn't see before. I'll work on that, and do the mapping.
@jhonndabi can you resolve the conflicts?
I think this is good to merge now once the conflicts are resolved.
@tsloughter yes, of course! Sorry the delay. : )
@tsloughter Is there something else I can do here to get this merged?
@jhonndabi hey, sorry, can you rebase?
And @yordis does this overlap with your PR that is still a draft?
@tsloughter yeah ... that is why I kept it that way, we can merge this one and I can continue the work there (either close it or change it)
@jhonndabi can you rebase this?
@tsloughter done : )