feat(otel): create a meter #1073
Conversation
SCKevinO
force-pushed
the
feat/1063/otel-sdk-create-meter
branch
from
c5d8b20 to
a645f4a
Compare
otel/metrics.go
Outdated
| ) | ||
|
|
||
| type Meter interface { | ||
| GetOriginalMeter() sdkmetric.Meter |
There was a problem hiding this comment.
We don't need this GetOriginalMeter(). The interface is needed for the next PR to list all methods to create instruments
| GetOriginalMeter() sdkmetric.Meter |
otel/metrics.go
Outdated
| func (m *meter) GetOriginalMeter() sdkmetric.Meter { | ||
| return m.sdkMeter | ||
| } |
There was a problem hiding this comment.
We don't need this. As instruments methods that will be implemented in the next PR, will use the meter internally through the struct. And i'm not sure we need to get this meter outside for now
There was a problem hiding this comment.
It's just a way to escape the abstraction if needed. That way, if for some reason, the methods we will implement are not enough, we can still get a meter and use the official sdk.
It should not be used in most cases but I think it's better to have it, just in case
There was a problem hiding this comment.
Yes but we could add it later if we need it. Actually keeping it minimal is better
There was a problem hiding this comment.
I'd say this PR is already pretty minimal as it is. Why remove this one-line function to add it back later? The otel sdk is quite complex and I can't pretend that we will handle every use case in this project, so an escape hatch can be useful
There was a problem hiding this comment.
I think it would prefer to not mix our otel package with the one of opentelemetry now. If it's not used properly later, it could add some complexity.
We need a minimal package that works the way we want it to. The marginal cases we'll see later. The goal for now is to have a working package.
It should be simple and easy to use, so for now i don't want to keep this as we don't have a case where use it for now. And honestly the name of the method doesn't satisfy me also.
There was a problem hiding this comment.
Fine I'll remove it. I don't agree with this but I don't want to spend all day arguing about it...
There was a problem hiding this comment.
For now, just make this package work as we want. We will see that later
SCKevinO
force-pushed
the
feat/1063/otel-sdk-create-meter
branch
from
a645f4a to
3a5de15
Compare
SCKevinO
force-pushed
the
feat/1063/otel-sdk-create-meter
branch
from
dfc504b to
a557926
Compare
Fix #1063
CHANGELOG.md