Emit spans for sdk #156
Emit spans for sdk #156
Conversation
|
blocked on Azure/go-autorest#335. Ready to go after that is merged. |
7fc926b to
3af0f3f
Compare
| @@ -31,6 +32,11 @@ func NewPetsClientWithBaseURI(baseURI string) PetsClient { | |||
|
|
|||
| // CreateAPInProperties create a Pet which contains more properties than what is defined. | |||
| func (client PetsClient) CreateAPInProperties(ctx context.Context, createParameters PetAPInProperties) (result PetAPInProperties, err error) { | |||
| ctx = tracing.StartSpan(ctx, "generated/additional-properties/PetsClient.CreateAPInProperties") | |||
| defer func() { | |||
| sc := result.StatusCode | |||
There was a problem hiding this comment.
We don't need a nil check here?
There was a problem hiding this comment.
yes discovered this while testing, will add a nill check and use resp instead of result
3af0f3f to
5cb2518
Compare
33fa7ab to
da59bee
Compare
|
@jhendrixMSFT regenerated the whole SDK, everything look good, no files missing or extra and only change is what is expected. And now we have nil checks everywhere so no chance of nil issues in case of no response |
| @@ -31,6 +32,14 @@ func NewPetsClientWithBaseURI(baseURI string) PetsClient { | |||
|
|
|||
| // CreateAPInProperties create a Pet which contains more properties than what is defined. | |||
| func (client PetsClient) CreateAPInProperties(ctx context.Context, createParameters PetAPInProperties) (result PetAPInProperties, err error) { | |||
| ctx = tracing.StartSpan(ctx, "generated/additional-properties/PetsClient.CreateAPInProperties") | |||
There was a problem hiding this comment.
What does the generated string look like for the SDK?
There was a problem hiding this comment.
its going to be the fully qualified name
There was a problem hiding this comment.
btw not the most happy on how we determine that. pretty much we take the outputpath and do an index on it
There was a problem hiding this comment.
I see so something like services/compute/mgmt/2016-03-30/compute/VirtualMachinesClient.CreateOrUpdate then?
There was a problem hiding this comment.
yes, here is a sample, https://gist.github.com/vladbarosan/3ab6cd696e9b7413194ab78ed75225c3 . Was wondering actually, I could prefix with github.com/Azure/Azure-sdk-for-go
There was a problem hiding this comment.
Good idea. It might be best to emit the package import path as an unexported const then concatenate that with the fully qualified method name.
| @@ -91,6 +100,14 @@ func (client PetsClient) CreateAPInPropertiesResponder(resp *http.Response) (res | |||
|
|
|||
| // CreateAPInPropertiesWithAPString create a Pet which contains more properties than what is defined. | |||
| func (client PetsClient) CreateAPInPropertiesWithAPString(ctx context.Context, createParameters PetAPInPropertiesWithAPString) (result PetAPInPropertiesWithAPString, err error) { | |||
| ctx = tracing.StartSpan(ctx, "generated/additional-properties/PetsClient.CreateAPInPropertiesWithAPString") | |||
There was a problem hiding this comment.
Per offline discussion please update codegen to not do any of this work if tracing.Enabled() == false.
| // Not necessary to perform this check as nothing will be instrumented if it is false, but | ||
| // adding it to avoid any potential perf issue. | ||
| if tracing.IsEnabled() { | ||
| ctx = tracing.StartSpan(ctx, fmt.Sprintf("%s/PetsClient.CreateAPInProperties", fqdn)) |
There was a problem hiding this comment.
Nit, I would simply do fqdn + "/PetsClient.CreateAPInProperties".
| @@ -31,6 +33,18 @@ func NewPetsClientWithBaseURI(baseURI string) PetsClient { | |||
|
|
|||
| // CreateAPInProperties create a Pet which contains more properties than what is defined. | |||
| func (client PetsClient) CreateAPInProperties(ctx context.Context, createParameters PetAPInProperties) (result PetAPInProperties, err error) { | |||
| // Not necessary to perform this check as nothing will be instrumented if it is false, but | |||
There was a problem hiding this comment.
Hmm I'm wondering if this comment should be removed. It adds two lines per API making the files longer and it's likely nobody will ever read it.
There was a problem hiding this comment.
well my reason was just not to make people think they need to do an isEnabled() check. If you think is overkill I can remove it
There was a problem hiding this comment.
and let me know if there is anything else
There was a problem hiding this comment.
thought about it a little, and yea seems a little overkill, makes more sense maybe to add this note in the go-autorest functions directly, so removing it
|
@jhendrixMSFT updated |
🤖 AutoRest automatic publish job 🤖success (version: 2.1.117) |
Fixes Azure/azure-sdk-for-go#2902
NextWithContext()for iterator and page types.