Skip to content

Add missing tenantid documentid #24795

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

Conversation

sonalideshpandemsft
Copy link
Contributor

@sonalideshpandemsft sonalideshpandemsft commented Jun 9, 2025

This PR ensures that tenant ID and document ID are captured and logged even in cases where the server times out or returns a 499 error. Previously, these values were missing as shown in the below query

https://dataexplorer.azure.com/clusters/frspme.westus2/databases/FRS?query=H4sIAAAAAAAAA1WOQYvCQAyF7%2F6K0JOCIHsV6mVB9OIu1j8QZ1I70JnpJpmKsj9%2BOy2uekrIe9972R6rnWp3pJ9EojL7hWtDTEA9BT05PxzRd7ApAS9x%2FmEX744DeoKyhOIlpAAMFjxpE%2B2ofX9Vp%2BKfE%2BLemYnCtmayE0ChdxyDH1JHreNok1EXw5PtUJtPVLpEvo2mlY0mZURWa6WAQff2pSoZQyJ1arO5xlboqSlqkjxY5eq0gaIaPiPOuCTvkd2dwMQUdL6A8w0e%2BUt4lOZ9yvkDp%2FRrfEcBAAA%3D

FRSHttpRequests
| where eventTimestamp >= ago(1d)
| where eventName == "HttpRequest" and method == "POST"
| where service == "alfred" and environment == "production"
| where pathCategory == "/documents/:tenantId"
| where successful == false
| where status startswith "Server"
| summarize count() by tenantId, documentId, status

@github-actions github-actions bot added base: main PRs targeted against main branch area: server Server related issues (routerlicious) labels Jun 9, 2025
): string | undefined {
let token: string | undefined;
if (authorization) {
const base64TokenMatch = authorization.match(/Basic (.+)/);
Copy link
Contributor

Choose a reason for hiding this comment

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

How did you get this logic? Thanks. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

On service side, this file is used for token validation logic

export function extractTokenFromHeader(authorizationHeader: string): string {

Copy link
Contributor

Choose a reason for hiding this comment

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

As I mentioned in below comment, alfred and historian have different token format. So it could break existing code.

): string | undefined {
let token: string | undefined;
if (authorization) {
const base64TokenMatch = authorization.match(/Basic (.+)/);

Choose a reason for hiding this comment

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

In addition to "basic", it can also be "Bearer"

@sonalideshpandemsft sonalideshpandemsft force-pushed the add-tenantid-documentid-1 branch from 966e215 to 691c7a9 Compare June 11, 2025 15:27
@sonalideshpandemsft sonalideshpandemsft marked this pull request as ready for review June 11, 2025 15:28
@sonalideshpandemsft sonalideshpandemsft changed the title Add tenantid documentid Add missing tenantid documentid Jun 11, 2025
Copy link
Contributor

@znewton znewton left a comment

Choose a reason for hiding this comment

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

A few perf related concerns here, especially with regards to a recent testing result that revealed performance degradations potentially caused by parsing tokens in this way.

@znewton znewton self-requested a review June 11, 2025 17:42
Copy link
Contributor

@znewton znewton left a comment

Choose a reason for hiding this comment

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

Misclicked approve. Requesting changes to reset

@sonalideshpandemsft sonalideshpandemsft force-pushed the add-tenantid-documentid-1 branch from 24a8867 to 47ebb48 Compare July 2, 2025 21:31
@sonalideshpandemsft sonalideshpandemsft enabled auto-merge (squash) July 3, 2025 20:38
@sonalideshpandemsft sonalideshpandemsft merged commit e987624 into microsoft:main Jul 3, 2025
32 checks passed
@sonalideshpandemsft sonalideshpandemsft deleted the add-tenantid-documentid-1 branch July 3, 2025 20:42
MarioJGMsoft pushed a commit to MarioJGMsoft/FluidFramework that referenced this pull request Jul 8, 2025
This PR ensures that tenant ID and document ID are captured and logged
even in cases where the server times out or returns a 499 error.
Previously, these values were missing as shown in the below query


https://dataexplorer.azure.com/clusters/frspme.westus2/databases/FRS?query=H4sIAAAAAAAAA1WOQYvCQAyF7%2F6K0JOCIHsV6mVB9OIu1j8QZ1I70JnpJpmKsj9%2BOy2uekrIe9972R6rnWp3pJ9EojL7hWtDTEA9BT05PxzRd7ApAS9x%2FmEX744DeoKyhOIlpAAMFjxpE%2B2ofX9Vp%2BKfE%2BLemYnCtmayE0ChdxyDH1JHreNok1EXw5PtUJtPVLpEvo2mlY0mZURWa6WAQff2pSoZQyJ1arO5xlboqSlqkjxY5eq0gaIaPiPOuCTvkd2dwMQUdL6A8w0e%2BUt4lOZ9yvkDp%2FRrfEcBAAA%3D

FRSHttpRequests
| where eventTimestamp >= ago(1d)
| where eventName == "HttpRequest" and method == "POST"
| where service == "alfred" and environment == "production"
| where pathCategory == "/documents/:tenantId"
| where successful == false
| where status startswith "Server"
| summarize count() by tenantId, documentId, status

---------

Co-authored-by: Sonali Deshpande <sonalideshpande@WIN-713HU8BVS0L.redmond.corp.microsoft.com>
Co-authored-by: Sonali Deshpande <sdeshpande@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Server related issues (routerlicious) base: main PRs targeted against main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants