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

Set all azcosmos telemetry spans to have the Kind of SpanKindClient #23618

Merged
merged 2 commits into from
Oct 23, 2024

Conversation

kjg
Copy link
Contributor

@kjg kjg commented Oct 21, 2024

Many tracing tools use the kind of the telemetry span to show the span in a specific way. By setting the azcosmos spans to type of Client, they will allow telemetry tools to treat the spans as the span of a database Client. When a span kind is not specified it is default to a kind of "Internal" which shows up to tracing tools as if these spans are part of internal code logic instead of spans generated by database client.

I've bumped azcore to v1.16.0, because that's the first version of azcore that allows the Span kind to be specified in the StartSpanOptions.

  • The purpose of this PR is explained in this or a referenced issue.
  • The PR does not update generated files.
  • Tests are included and/or updated for code changes.
  • Updates to module CHANGELOG.md are included.
  • MIT license headers are included in each file.

Copy link

Thank you for your contribution @kjg! We will review the pull request and get back to you soon.

@github-actions github-actions bot added Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Oct 21, 2024
@kjg
Copy link
Contributor Author

kjg commented Oct 21, 2024

The lint errors on this PR appear to be unrelated to the changes, should I take any action on them?

@Pilchie
Copy link
Member

Pilchie commented Oct 21, 2024

@simorenoh @analogrelay - can one of you take a look at this.

@analogrelay
Copy link
Member

The lint errors on this PR appear to be unrelated to the changes, should I take any action on them?

I believe you're correct, they're unrelated, but I'd love to get us green on this PR before we land it. Let me take a look and see if it's a straightforward fix. If it is, it'd be good to get it in here.

@analogrelay
Copy link
Member

analogrelay commented Oct 22, 2024

Yeah, it's a straightforward fix, just removing the nil checks here:

cosmos_item_request_options.go:45:5: S1009: should omit nil check; len() for []string is defined as zero (gosimple)
	if options.PreTriggers != nil && len(options.PreTriggers) > 0 {
	   ^
cosmos_item_request_options.go:49:5: S1009: should omit nil check; len() for []string is defined as zero (gosimple)
	if options.PostTriggers != nil && len(options.PostTriggers) > 0 {
	   ^
cosmos_location_cache_test.go:186:5: S1009: should omit nil check; len() for []github.com/Azure/azure-sdk-for-go/sdk/data/azcosmos.accountRegion is defined as zero (gosimple)
	if dbAcct.WriteRegions == nil || len(dbAcct.WriteRegions) == 0 {

I haven't yet been able to track down why these weren't failing before. There was a change to this file last month that built successfully, and as far as I can tell, we haven't changed our golangci-lint config since then. I'll do a little looking, but the fix should be safe and straightforward. len(nil) is safe to call and returns 0, which works for all these cases. @kjg would you mind making that change here to get us green? I'm also happy to add it to your PR if the permissions are set up for that, or make the change in a separate PR to main that we land first, whatever works for you. We don't need this check to merge but I don't like merging with red checks unless there's a very good reason to :).

@kjg
Copy link
Contributor Author

kjg commented Oct 22, 2024

I'll make it here, Thanks!

Copy link
Member

@analogrelay analogrelay left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'd like the previously-failing lint check to pass before merging (though it's not directly related to the main PR).

@analogrelay
Copy link
Member

I haven't yet been able to track down why these weren't failing before.

Looks like there was an update of our golangci version in #23536 that was green in the PR (because that PR didn't build all the packages) but failed in main because of the linter update. That likely explains why the lint started failing without a specific PR to cause it.

@kjg
Copy link
Contributor Author

kjg commented Oct 23, 2024

It looks like all the tests are passing now, so I think this is good to go. Thanks for the guidance!

@analogrelay
Copy link
Member

@kirankumarkolli @Pilchie @simorenoh @kushagraThapar , I'm not a CODEOWNER yet. Could one of you take a look at this?

@Pilchie Pilchie merged commit 5a97c43 into Azure:main Oct 23, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Contribution Community members are working on the issue Cosmos customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants