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.

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

Switching log from debug to warning #471

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Switching log from debug to warning #471

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e620f9
Switching log from debug to warning
rossgrambo Jul 9, 2024
bfdba17
Adjusted warning logs
rossgrambo Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Microsoft.FeatureManagement.AspNetCore/TargetingHttpContextMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,11 @@ public async Task InvokeAsync(HttpContext httpContext, ITargetingContextAccessor

if (activityFeature == null)
{
_logger.LogDebug("The IHttpActivityFeature from the IFeatureCollection was null");
_logger.LogWarning("The IHttpActivityFeature from the IFeatureCollection was null");
Copy link
Member

Choose a reason for hiding this comment

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

If a customer came to us and asked us what they should do with this warning, what would we suggest? Can we make the message more helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good question, and kinda no. The only circumstances I'm aware of where this can happen is if some code before ours is ripping out the IHttpActivityFeature from the ServiceCollection that's included by default. Or if we're in the context of an integration test or something where the middleware is called directly with a manually created HttpContext.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the only way I can imagine it would be missing is if someone/something running earlier on in the request ripped it out.

Could probably say "A request feature required for telemetry, IHttpActivityFeature, was not found in the request's feature collection. Removing this feature from the request's feature collection will cause telemetry emission to fail."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's reasonable. I'll update to that format

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to

if (activityFeature == null)
{
    _logger.LogWarning("A request feature required for telemetry, IHttpActivityFeature, was not found in the request's feature collection. Removing this feature from the request's feature collection will cause telemetry emission to fail.");
}
else if (activityFeature.Activity == null)
{
    _logger.LogWarning("The IHttpActivityFeature request feature's Activity is null. This will cause telemetry emission to fail.");
}

Copy link
Member

Choose a reason for hiding this comment

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

_logger.LogWarning("The IHttpActivityFeature request feature's Activity is null. This will cause telemetry emission to fail.");

Can we update this to provide more context like the other too?

Copy link
Contributor Author

@rossgrambo rossgrambo Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

How about:

_logger.LogWarning("The Activity on the IHttpActivityFeature request feature was null. The missing activity will cause telemetry emission to fail.")

?

I think the situation is if IHttpActivityFeature was manually added/overridden and Activity was not set- but having trouble turning that into an error message without assuming too much.

Copy link
Member

Choose a reason for hiding this comment

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

Give users some context of what IHttpActivityFeature is about. Suggest below if it makes sense to you:

A request feature required for telemetry, IHttpActivityFeature, has a null Activity property. If you have updated IHttpActivityFeature, ensure the Activity property is not null. A missing Activity will result in telemetry emission failures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

}
else if (activityFeature.Activity == null)
{
_logger.LogDebug("The Activity on the IHttpActivityFeature was null");
_logger.LogWarning("The Activity on the IHttpActivityFeature was null");
}
else
{
Expand Down