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

Global initialization and resources proposal: demonstration #3

Closed
wants to merge 14 commits into from

Conversation

jmacd
Copy link
Owner

@jmacd jmacd commented Jul 29, 2019

This is not meant to merge as a single PR, as it contains too many separate parts (which should be tested). This is meant to show the final state referred to in several RFCs with respect to initializing globals and setting up resources.

I expect this does not go far enough, probably we'll want to introduce some sort of Tracer/Meter builder concept, as discussed here: open-telemetry/opentelemetry-specification#10 (comment)

func (k Key) Defined() bool {
return k.Variable.Defined()
}

// TODO make this a lazy one-time conversion.
func (v Value) Evaluate() Value {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue of making a copy of the Value object was brought up in the #59 too. So just to keep track of that I'll mention it here: the BYTES case should also be handled here by making a new slice and copying a contents of the original slice into the new slice.

@@ -26,8 +43,8 @@ type Value struct {
Float64 float64
String string
Bytes []byte

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a type that we have in the OpenTelemetry specs.

@@ -164,8 +222,10 @@ func (v Value) Emit() string {
return fmt.Sprint(v.Float64)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we only emit strings?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants