Skip to content

Conversation

@soninaren
Copy link
Member

@soninaren soninaren commented Feb 5, 2020

@soninaren soninaren requested a review from brettsam February 5, 2020 20:37
@soninaren
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

}


private static async Task<string> AddLoggingConfig(string hostJsonContent)
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason for this approach as opposed to just have it in the default host.json?

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the sample of setting bundles as default in host.json. I think the only advantage we get by following this approach, is that we can introduce a flag to skip this logic in the init command no-loggingConfig. While that made sense for bundles, I don't think it does for Application Insight configuration. I will revert this change to simple add this configuration to default host.json

Copy link
Contributor

Choose a reason for hiding this comment

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

sounds good

@brettsam
Copy link
Member

While you are doing this -- @jeffhollan (or @paulbatum... I can't remember who brought it up), weren't we discussing also adding this to the default?

"samplingSettings": {
    "isEnabled": true 
}

So it'd be much more "in your face" that sampling is enabled by default?

@soninaren
Copy link
Member Author

While you are doing this -- @jeffhollan (or @paulbatum... I can't remember who brought it up), weren't we discussing also adding this to the default?

"samplingSettings": {
    "isEnabled": true 
}

So it'd be much more "in your face" that sampling is enabled by default?

Jeff added that to the issue after the PR was out. Will add that to the config as well

@soninaren
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@soninaren
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@kashimiz kashimiz left a comment

Choose a reason for hiding this comment

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

:shipit:

@soninaren soninaren merged commit a4e2578 into dev Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Exclude sampling of requests by default in host.json templates

6 participants