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

Cosmos DB: Network Level Metrics #1543

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

sourabh1007
Copy link
Contributor

Changes

Ref. #1495

Note: if the PR is touching an area that is not listed in the existing areas, or the area does not have sufficient domain experts coverage, the PR might be tagged as experts needed and move slowly until experts are identified.

Merge requirement checklist

@sourabh1007 sourabh1007 requested review from a team as code owners November 1, 2024 21:14
@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/networklevelmetrics branch from 1f67275 to f7bbc13 Compare November 4, 2024 02:23
Copy link

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

Left few comments

model/database/registry.yaml Outdated Show resolved Hide resolved
model/database/registry.yaml Outdated Show resolved Hide resolved
model/database/registry.yaml Outdated Show resolved Hide resolved
model/database/registry.yaml Show resolved Hide resolved
model/database/registry.yaml Outdated Show resolved Hide resolved
model/database/cosmosdb-metrics.yaml Outdated Show resolved Hide resolved
model/database/cosmosdb-metrics.yaml Outdated Show resolved Hide resolved
model/database/common.yaml Outdated Show resolved Hide resolved
model/database/cosmosdb-metrics.yaml Outdated Show resolved Hide resolved
model/database/cosmosdb-metrics.yaml Outdated Show resolved Hide resolved
@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/networklevelmetrics branch from 6592787 to 762190b Compare November 21, 2024 05:57
@sourabh1007 sourabh1007 force-pushed the users/sourabhjain/networklevelmetrics branch from fe03b44 to 0926a76 Compare November 21, 2024 07:36
Copy link

@FabianMeiswinkel FabianMeiswinkel left a comment

Choose a reason for hiding this comment

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

One blocking comment left

| [`db.cosmosdb.network.response.status_code`](/docs/attributes-registry/db.md) | int | The status code returned in response. [2] | `200` | `Conditionally Required` If available | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| [`db.cosmosdb.network.response.sub_status_code`](/docs/attributes-registry/db.md) | int | The Azure Cosmos DB sub-status code for a network request made during an operation. | `1002` | `Conditionally Required` If available | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| [`db.cosmosdb.sub_status_code`](/docs/attributes-registry/db.md) | int | The Azure Cosmos DB sub-status code for an operation represents the final sub-status code after multiple network interactions have been completed during the operation. | `1000`; `1002` | `Conditionally Required` when response was received and contained sub-code. | ![Experimental](https://img.shields.io/badge/-experimental-blue) |
| [`db.namespace`](/docs/attributes-registry/db.md) | string | The name of the database, fully qualified within the server address and port. | `customers`; `test.users` | `Conditionally Required` If available. | ![Experimental](https://img.shields.io/badge/-experimental-blue) |

Choose a reason for hiding this comment

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

I still think you are missing a place to capture the CosmosDBAccountName

IMO teh namespace should be a concatenation of Account and Database

Why? The server-addresss (the Gateway endpoint maps to DocuemtnServiceId) - it is the DNS-name for an account - this can be different and not be prefixed with CosmsoDBAccount - it is not super common - but technically those are two different settings - and there are real accounts where server-address does not start with account name - and given that any support/debugging will need the account name, I think emitting this would be needed?

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

Successfully merging this pull request may close these issues.

4 participants