Where communities thrive


  • Join over 1.5M+ people
  • Join over 100K+ communities
  • Free without limits
  • Create your own community
People
Activity
    Joshua MacDonald
    @jmacd
    Luis
    @lmineiro
    It seems that the Resources, at least for the Tracer, are a good method to provide what used to be "Global Tags" -- tags that were set on every Span, defined once at startup time
    Branden Horiuchi
    @bhoriuchi
    Can someone point me in the right direction to capture errors in my traces?
    Branden Horiuchi
    @bhoriuchi
    Do I just set the code on the span and add an attribute for the message?
    Tristan Sloughter
    @tsloughter
    hi, when looking over the Go code I noticed the use of runtime/trace, curious how this is used. is it so otel trace data can be exported as regular Go trace information for analysis? it looks like it is on by default?
    Tyler Yahn
    @MrAlias
    @tsloughter : I'm not 100%, but it looks like that is a holdover from Open Census. Guessing you are talking about https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/trace/trace_go11.go#L24, which looks like a debugging setup for validating behavior in Go1.11 (I'm guessing this was to evaluate the new module code introduced there)
    fwiw, that build flag in the file and the conditional means it is not on by default. To enable that you need build with Go 1.11 and pass the -trace CLI flag (IIRC)
    Tristan Sloughter
    @tsloughter
    aah
    ok, thanks. I guess thatmakes more sense. thought it could be interesting to combine debug tracing and otel tracing
    Joshua MacDonald
    @jmacd
    runtime/trace and runtime/pprof both have useful methods for adding information to the Go execution tracer profile and the pprof dumps. They made (IMO) mistakes which make it impossible to build distributed tracing with, e.g., func WithRegion(ctx context.Context, regionType string, fn func()) does not pass a child context into fn. I see reasonable tracer SDK configurations that would introduce calls to those methods as a side-effect of trace.Start and trace.WithSpan.
    Tyler Yahn
    @MrAlias
    I created open-telemetry/opentelemetry-go#429 to track the work of removing the tracer name prefix from span names.
    I'm going to try and get a PR together this afternoon to address this (wasn't able to assign it to myself).
    Unless, someone else is already jammin on this...
    Gustavo Silva Paiva
    @paivagustavo
    I've assigned it to you @MrAlias ! Thanks for filling it :)
    Tyler Yahn
    @MrAlias
    Thanks :)
    Tyler Yahn
    @MrAlias
    I noticed that the metric SDK doesn't implement the metric Provider interface (missing the Meter(name string) Meter method).
    Is this intentional? Is there contention in me flushing this out?
    @jmacd : I saw you removed that Provider interface in your "Resource Scope and Namespace" proposal. Did you have goals regarding this topic?
    Joshua MacDonald
    @jmacd
    The "push" controller implements the interface, although it is a No-op.
    Tyler Yahn
    @MrAlias
    ah, gotcha. Thanks for the help
    Tyler Yahn
    @MrAlias
    @jmacd : is there a reasoning the SDK metric export package is called export and not metric? https://github.com/open-telemetry/opentelemetry-go/blob/master/sdk/export/metric/metric.go#L15
    Seems a bit clunky from the user perspective to import "go.opentelemetry.io/otel/sdk/export/metric" and have it imported as export instead.
    Joshua MacDonald
    @jmacd
    The user is more likely to import the api/metric package, I figure.
    Bogdan Drutu
    @bogdandrutu
    maybe use a longer name metricexport
    Tyler Yahn
    @MrAlias
    I was looking at it from the exporter writer's perspective (looking at the stdout exporter currently).
    Would there be any opposition to renaming it to metric to match the "go.opentelemetry.io/otel/sdk/export/trace" package (which is named trace)?
    The only issue I saw was the MetricKind would need to be renamed (to not have golint complain)
    Joshua MacDonald
    @jmacd
    OHhhhhh I see what you mean. I wasn't even aware that it was named "export". It should be fixed.
    Tyler Yahn
    @MrAlias
    :thumbsup: I'll see about getting a PR together this afternoon
    Joshua MacDonald
    @jmacd
    I moved that package at some point--it had been named "export". I rewrote all the import statements to rename the package "export" everywhere, thus didn't notice I hadn't renamed the package.
    Tyler Yahn
    @MrAlias
    Ah, gotcha. I was guessing something like that ^, but wanted to check that it wasn't an important choice that was made.
    Joshua MacDonald
    @jmacd

    Folks I can't make it to the SIG meeting today. I took a glance at @paivagustavo's latest PR for improving histogram concurrency and didn't have a chance to finish. I recall there was a call to runtime.Gosched() in the Prometheus code that was to let collection block while writers finish. @paivagustavo it looks like you've done something slightly different?

    I will be hosting the Metrics SIG meeting if anyone wants to discuss this ^^^ at 11am PST.

    Gustavo Silva Paiva
    @paivagustavo
    Yeah, I've purposely left out that part, it is just a simplified version and that is mostly because Prometheus don't reset metrics at collect and thus needs to wait every writers finishes to create a new state that keep all previously recorded data. And they also collect instantly.
    I'll join the Metrics SIG meeting to discuss and explain it
    Nathanial Murphy
    @neetle
    Hey guys, would here be the right place to spitball a Go SDK API proposition before submitting an issue on github?
    Tyler Yahn
    @MrAlias
    @neetle : possibly. The Go SDK API is somewhat tied to the overall OpenTelemetry proposed design for it.
    What is your proposal? Would be able to better determine a good form from that
    Nathanial Murphy
    @neetle
    It's simple, but the justification is complex. So.. bare with me.
    Currently all of the metric.NewT(Counter|Gauge|Measure) and metric.T(Counter|Gauge|Measure).Bind() return non-pointer structs. For example, metric.NewFloat64Measure() returns a Float64Measure and func (*Float64) Bind() BoundFloat64Measure.
    Nathanial Murphy
    @neetle

    However, all the methods on the returned structs have pointer receivers, so you need to take the address of the result before you can call the next method. This leads to code like
    myMeasure := metric.NewFloat64()

    myBoundMeasure := myMeasure.Bind() // implied pointer to myMeasure here

    myStruct := struct{ measure *metric.BoundFloat64Measure } { measure: &myBoundMeasure, // explicit pointer to myBoundMeasure }

    Given that the zero value for these methods is also unsafe to use as there's no way to check if it's not initialized, I'd propose that metric.NewT(Counter|Gauge|Measure) and all associated Bind() methods return a pointer rather than a raw struct
    that was a lot simpler than I expected
    Given the fact that this is only really dealing with go-isms, I figure that this wouldn't deviate from the spirit of otel that much
    Joshua MacDonald
    @jmacd
    Hmm -- the goal was to avoid memory allocations. You can embed one struct into another and call pointer methods, since the field is addressable in the sense of https://golang.org/pkg/reflect/#Value.CanAddr
    At the same time, those methods should probably not use pointer receivers.
    Tyler Yahn
    @MrAlias
    +1 on not using pointer receivers if they are not changing the struct
    Nathanial Murphy
    @neetle
    Yeah, removing the pointer receivers should also do the trick
    just bringing the returns into line with the receivers would work
    Tyler Yahn
    @MrAlias
    @neetle : did you want to create a issue to track that task?
    Nathanial Murphy
    @neetle
    Yeah, I'll throw one together
    Tyler Yahn
    @MrAlias
    (also, thanks for bringing it up :) )
    Nathanial Murphy
    @neetle
    np, most projects need someone to complain to get it improved
    Joshua MacDonald
    @jmacd
    Yes, thank you :)
    Nathanial Murphy
    @neetle
    the only real issue I see with the suggested solution is that there's no way to confirm that the measures have been setup, and using them before they've been set up causes panics
    but we can sort that on the issue
    Tyler Yahn
    @MrAlias
    +1 ^
    Nathanial Murphy
    @neetle
    Alright, for those playing at home: open-telemetry/opentelemetry-go#440
    Thanks again guys!
    Liz Fong-Jones
    @lizthegrey
    I passed this to @rebeccajae as a potential second issue to work on :)