Skip to content

Client Level Diagnostics for Spring SDK#35546

Merged
trande4884 merged 22 commits into
Azure:mainfrom
trande4884:trande_clientLevelDiagnostics
Jul 8, 2023
Merged

Client Level Diagnostics for Spring SDK#35546
trande4884 merged 22 commits into
Azure:mainfrom
trande4884:trande_clientLevelDiagnostics

Conversation

@trande4884

@trande4884 trande4884 commented Jun 20, 2023

Copy link
Copy Markdown
Member

Description

Adding threshold support for diagnostics at the client level.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
  • CHANGELOG is updated for new features, bug fixes or other significant changes.
  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

This reverts commit 6bab94e.
@azure-sdk

Copy link
Copy Markdown
Collaborator

API change check

API changes are not detected in this pull request.

@trande4884

Copy link
Copy Markdown
Member Author

No test cases to commit for this change, but I have confirmed with local testing that the configured values are being used.
image

@trande4884 trande4884 marked this pull request as ready for review June 21, 2023 13:33
@trande4884 trande4884 requested a review from backwind1233 as a code owner June 21, 2023 13:33

@FabianMeiswinkel FabianMeiswinkel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please see comments about public surface area (configs)

@FabianMeiswinkel FabianMeiswinkel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM now

@FabianMeiswinkel

Copy link
Copy Markdown
Member

Would be nice to have a brief sample on how to enable logging with reaosnable defaults in readme.md - ultimately we want to enable everyone these logs, correct? Also makes it easier for support to point customers to a sample on how to enable them

@kushagraThapar kushagraThapar left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@trande4884 looks great, can we add more samples for this in the readme section?
Also, we have not added any tests yet. Can we add some tests to make sure this works end to end?

@trande4884

Copy link
Copy Markdown
Member Author

@trande4884 looks great, can we add more samples for this in the readme section? Also, we have not added any tests yet. Can we add some tests to make sure this works end to end?

I can add samples, but for these attributes added on the configuration there is no real way to test them usually, I did manually test to make sure the values were being passed along. Is there some specific aspect I should be trying to test?

@kushagraThapar

Copy link
Copy Markdown
Member

@trande4884 looks great, can we add more samples for this in the readme section? Also, we have not added any tests yet. Can we add some tests to make sure this works end to end?

I can add samples, but for these attributes added on the configuration there is no real way to test them usually, I did manually test to make sure the values were being passed along. Is there some specific aspect I should be trying to test?

Is there a way to test using DiagnosticsHandler? - checking if the APIs are getting called when thresholds are met?

@trande4884

Copy link
Copy Markdown
Member Author

@kushagraThapar @FabianMeiswinkel more information added to readme as well as a test case.

@trande4884

Copy link
Copy Markdown
Member Author

/azp run java - spring - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

Comment thread sdk/spring/azure-spring-data-cosmos/CHANGELOG.md Outdated
Comment thread sdk/spring/azure-spring-data-cosmos/README.md Outdated
trande4884 and others added 2 commits July 7, 2023 16:11
Changing wording.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
Wording change.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
@trande4884

Copy link
Copy Markdown
Member Author

/azp run java - spring - tests

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@trande4884 trande4884 merged commit 705fa70 into Azure:main Jul 8, 2023
trande4884 added a commit to trande4884/azure-sdk-for-java-trande that referenced this pull request Jul 27, 2023
* Merging

* Revert "Merging"

This reverts commit ade7932.

* Exposing diagnostic threshold's at the client level in Spring SDK.

* Revert "Revert "Merging""

This reverts commit 6bab94e.

* Updating changelog.

* Removing unused import.

* Adding defaults to application.properties

* Fixing copy/paste error in code.

* Fixing PR comments.

* Fixing PR comments.

* Fixing PR comments.

* Adding test case.

* Adding javaagent sample to readme.

* Updating readme wording.

* Removing unused imports.

* Fixing bad merge resolution.

* Update sdk/spring/azure-spring-data-cosmos/CHANGELOG.md

Changing wording.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

* Update sdk/spring/azure-spring-data-cosmos/README.md

Wording change.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

---------

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
trande4884 added a commit that referenced this pull request Jul 27, 2023
* Client Level Diagnostics for Spring SDK (#35546)

* Merging

* Revert "Merging"

This reverts commit ade7932.

* Exposing diagnostic threshold's at the client level in Spring SDK.

* Revert "Revert "Merging""

This reverts commit 6bab94e.

* Updating changelog.

* Removing unused import.

* Adding defaults to application.properties

* Fixing copy/paste error in code.

* Fixing PR comments.

* Fixing PR comments.

* Fixing PR comments.

* Adding test case.

* Adding javaagent sample to readme.

* Updating readme wording.

* Removing unused imports.

* Fixing bad merge resolution.

* Update sdk/spring/azure-spring-data-cosmos/CHANGELOG.md

Changing wording.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

* Update sdk/spring/azure-spring-data-cosmos/README.md

Wording change.

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

---------

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

* Update sdk/spring/azure-spring-data-cosmos/CHANGELOG.md

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>

* Update README.md

Adding readme file changes.

Co-Authored-By: Kushagra Thapar <kushuthapar@gmail.com>

* Update README.md

Adding changes to readme file

---------

Co-authored-by: Kushagra Thapar <kushuthapar@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

azure-spring All azure-spring related issues Cosmos

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants