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

Initial pr #1

Closed
wants to merge 23 commits into from
Closed

Initial pr #1

wants to merge 23 commits into from

Conversation

dawallin
Copy link
Contributor

@dawallin dawallin commented Jun 6, 2016

Here is the first pull request so we can get something to discuss around.

@bhs
Copy link

bhs commented Jun 7, 2016

nit (while looking at the file list): is NuGet.exe meant to be a part of the PR?

void Finish();
void FinishWithOptions(DateTime finishTime);

void SetBaggageItem(string restrictedKey, string value);
Copy link

Choose a reason for hiding this comment

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

unlike SetTag and Log, SetBaggageItem is meant to be paired with a GetBaggageItem...

also, I am fairly confident we are going to remove restrictions on the keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My thought to get the baggage was to do something like this.

var memoryCarrier = new MemoryTextMapCarrier(new Dictionary<string, string>() { });
tracer.Inject(span, memoryCarrier);            
var value = memoryCarrier.TextMap[key]

But I agree, a GetBaggage(key, value) method on the span is more convinient. I will add it.

Copy link

Choose a reason for hiding this comment

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

Thanks...

Also, a general point of order: I was inspired by your SpanContext stuff and am attempting to find a compromise where other languages can incorporate a context concept at the OpenTracing API layer without introducing tons of new terminology and cognitive load for the user. I will keep you informed of the RFC PR on that front which I hope to send out this weekend. (I've done the interesting part but need to clean things up)

@dawallin
Copy link
Contributor Author

dawallin commented Jun 11, 2016

About the nuget.exe. I'm looking at other OSS projects and sometimes I found it included. It is the artifact package builder. I change and exclude it for now and need to look up the best practice for this.

@bhs
Copy link

bhs commented Jun 11, 2016

@dawallin: somewhat related to the context stuff, see https://gitter.im/opentracing/public?at=575c92732eaa837d71e8a0fd

@cwe1ss
Copy link
Member

cwe1ss commented Aug 31, 2016

closed, because we merged #2

@cwe1ss cwe1ss closed this Aug 31, 2016
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