Skip to content

Logging improvements to DiagnosticEventTableStorageRepository #10996

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Apr 29, 2025
Merged

Logging improvements to DiagnosticEventTableStorageRepository #10996

merged 9 commits into from
Apr 29, 2025

Conversation

cjaliaga
Copy link
Member

@cjaliaga cjaliaga commented Apr 11, 2025 •
edited
Loading

Issue describing the changes in this PR

This PR introduces improvements to the logging and error handling mechanisms within the DiagnosticEventTableStorageRepository class. The main changes include the addition of a dedicated Logger class for structured logging and enhancements to error handling to disable the service when it's not able to connect to the table storage service.

Resolves #10757 #10995

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

@cjaliaga cjaliaga requested a review from a team as a code owner April 11, 2025 23:33
@cjaliaga
Copy link
Member Author

cjaliaga commented Apr 11, 2025 •
edited
Loading

TODO:
1) Run the private site extension to test it E2E
2) Add additional test coverage to test for permissions. Probably I'll need to use the Azurite Fixture to see if I can get a valid connection string with an invalid key to force a 403.

@cjaliaga
Moving logging to defined methods in DiagnosticEventsTableStorageRepo…
5891f6c 
…sitory. Disabling service when the client is not authorized to use the table storage service.
@cjaliaga cjaliaga force-pushed the cjaliaga/diagnotic-events-10757 branch from f993129 to 5891f6c Compare April 12, 2025 21:28
@RohitRanjanMS
Copy link
Member

#10995.

Do we have any publicly available documentation for this feature? If not, it would be helpful to create documentation outlining its behavior and the requirements needed to enable it.

@cjaliaga
Disable service when we fail to write events due to permissions.
0070a74 
Check first for initialized _tableClient.
@jviau jviau mentioned this pull request Apr 16, 2025
Open
jviau
jviau approved these changes Apr 25, 2025
View reviewed changes
Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

LGTM. One minor concern is if the issues leading to the service failing are resolved, instances need to be restarted to re-enable this service. I can live with that for now, but something for us to be aware of.

@cjaliaga
Copy link
Member Author

cjaliaga commented Apr 25, 2025 •
edited
Loading

LGTM. One minor concern is if the issues leading to the service failing are resolved, instances need to be restarted to re-enable this service. I can live with that for now, but something for us to be aware of.

I have the same concern but I'm ok for now as well. We could have a timer that runs the checks every 30 minutes for example (without logging errors of course) and re-enables the service if the issues are resolved.

@jviau
Copy link
Contributor

jviau commented Apr 28, 2025

LGTM. One minor concern is if the issues leading to the service failing are resolved, instances need to be restarted to re-enable this service. I can live with that for now, but something for us to be aware of.

I have the same concern but I'm ok for now as well. We could have a timer that runs the checks every 30 minutes for example (without logging errors of course) and re-enables the service if the issues are resolved.

That seems reasonable for a future improvement. I would want to make sure we don't log this as an error though, maybe a warning.

@cjaliaga cjaliaga requested a review from RohitRanjanMS April 29, 2025 22:16
@cjaliaga cjaliaga mentioned this pull request Apr 29, 2025
Merged
@cjaliaga cjaliaga merged commit 32c0e71 into dev Apr 29, 2025
9 checks passed
@cjaliaga cjaliaga deleted the cjaliaga/diagnotic-events-10757 branch April 29, 2025 23:46
soninaren pushed a commit that referenced this pull request May 1, 2025
@cjaliaga @soninaren
Logging improvements to DiagnosticEventTableStorageRepository (#10996)
5f82330
soninaren added a commit that referenced this pull request May 2, 2025
@soninaren @azfuncgh @cjaliaga
HotFix 4.39.500 (#11045)
f3334c3 
* Update version to 4.1039.500

* Clear release notes

* Logging improvements to DiagnosticEventTableStorageRepository  (#10996)

---------

Co-authored-by: Azure Functions Release <[email protected]>
Co-authored-by: Carlos J. Aliaga <[email protected]>
kazuyuk added a commit to kazuyuk/azure-docs that referenced this pull request May 7, 2025
@kazuyuk
Update functions-azurewebjobsstorage-permissions.md
dca56dd 
it need to perminssion for table storage account

Azure/azure-functions-host#10996
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kshyju kshyju kshyju approved these changes

@jviau jviau jviau approved these changes

@RohitRanjanMS RohitRanjanMS RohitRanjanMS approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve error handling when DiagnosticEventsTableRepository cannot flush logs to table storage
4 participants
@cjaliaga @RohitRanjanMS @jviau @kshyju