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

System.Diagnostics.DiagnosticSource: Activity with null operation name #77621

Closed
Tracked by #78518
CodeBlanch opened this issue Oct 28, 2022 · 2 comments · Fixed by #77746
Closed
Tracked by #78518

System.Diagnostics.DiagnosticSource: Activity with null operation name #77621

CodeBlanch opened this issue Oct 28, 2022 · 2 comments · Fixed by #77746
Assignees
Labels
area-System.Diagnostics.Activity breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Milestone

Comments

@CodeBlanch
Copy link
Contributor

Activity has a quirk in that its OperationName is not marked as nullable but its constructor allows it to be null. Chatted offline with the group CCed below and we agreed that it is a low-risk change to coalesce to string.Empty in this case.

Something like this:

        public Activity(string operationName)
        {
            Source = s_defaultSource;
            // Allow data by default in the constructor to keep the compatibility.
            IsAllDataRequested = true;

            if (string.IsNullOrEmpty(operationName))
            {
                NotifyError(new ArgumentException(SR.OperationNameInvalid)); // <- Note: Does not throw.
                operationName = string.Empty; // <- New logic here.
            }

            OperationName = operationName;
        }

We consider setting to something like "<null>" but decided to use string.Empty because that is similar to what happens for Activity.Source.Name in the case of legacy instances.

/cc @tarekgh @noahfalk @reyang @cijothomas @utpilla @alanwest

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Oct 28, 2022
@tarekgh tarekgh added area-System.Diagnostics.Activity and removed untriaged New issue has not been triaged by the area owner labels Oct 28, 2022
@ghost
Copy link

ghost commented Oct 28, 2022

Tagging subscribers to this area: @dotnet/area-system-diagnostics-activity
See info in area-owners.md if you want to be subscribed.

Issue Details

Activity has a quirk in that its OperationName is not marked as nullable but its constructor allows it to be null. Chatted offline with the group CCed below and we agreed that it is a low-risk change to coalesce to string.Empty in this case.

Something like this:

        public Activity(string operationName)
        {
            Source = s_defaultSource;
            // Allow data by default in the constructor to keep the compatibility.
            IsAllDataRequested = true;

            if (string.IsNullOrEmpty(operationName))
            {
                NotifyError(new ArgumentException(SR.OperationNameInvalid)); // <- Note: Does not throw.
                operationName = string.Empty; // <- New logic here.
            }

            OperationName = operationName;
        }

We consider setting to something like "<null>" but decided to use string.Empty because that is similar to what happens for Activity.Source.Name in the case of legacy instances.

/cc @tarekgh @noahfalk @reyang @cijothomas @utpilla @alanwest

Author: CodeBlanch
Assignees: -
Labels:

area-System.Diagnostics.Activity

Milestone: -

@tarekgh tarekgh added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Oct 28, 2022
@tarekgh tarekgh added this to the 8.0.0 milestone Oct 28, 2022
@tarekgh tarekgh added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 28, 2022
@tarekgh tarekgh self-assigned this Oct 28, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 1, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Activity breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants