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

Implement User-Agent spec for transport #1518

Merged
merged 15 commits into from
Nov 9, 2021

Conversation

apmmachine
Copy link
Contributor

@apmmachine apmmachine commented Oct 12, 2021

What

APM agent specs automatic sync

Why

Changeset

Closes #1517

@apmmachine
Copy link
Contributor Author

apmmachine commented Oct 12, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-11-09T00:27:10.500+0000

  • Duration: 89 min 0 sec

  • Commit: 778a4d0

Test stats 🧪

Test Results
Failed 0
Passed 20164
Skipped 133
Total 20297

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

@russcam russcam self-assigned this Nov 1, 2021
russcam and others added 6 commits November 1, 2021 18:31
This commit implements the User-Agent feature.
Update User-Agent sent by the agent to match the spec.
HttpClient parses apm-agent-dotnet/<version> (<service> <version>)
as two separate User-Agent header values,

apm-agent-dotnet/<version>
(<service> <version>)

- Refactor outcome steps to use ScenarioContext.
- Introduce TestConfiguration to allow configuration to be provided a step
at a time.
- Collect all payloads sent to mock HttpMessageHandler.
…/apm-agent-dotnet into update-spec-files-20211012142323
@russcam russcam requested a review from gregkalapos November 3, 2021 00:10
@russcam russcam changed the title test: synchronizing gherkin spec Implement User-Agent spec for transport Nov 5, 2021
Copy link
Contributor

@gregkalapos gregkalapos left a comment

Choose a reason for hiding this comment

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

LGTM

// Elasticsearch B.V licenses this file to you under the Apache 2.0 License.
// See the LICENSE file in the project root for more information

using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
using System;

[When("^service name is set to '(.*?)'$")]
public void WhenServiceNameIsSetTo(string name)
{
var configuration = _scenarioContext.Get<TestConfiguration>();
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit sad we can't just use Elastic.Apm.Tests.Utilities.MockConfiguration here.

Copy link
Contributor

Choose a reason for hiding this comment

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

MockConfiguration would require some work to factor it for usage here. I'm thinking

  1. Remove the parsing functions used in MockConfiguration, and test them separately
  2. Add public setters to the existing public properties
  3. Change the constructor to be parameterless and initialize field members with their default values

I considered making those changes for this PR, but it felt like they would be large enough to open a separate PR for.


namespace Elastic.Apm.Feature.Tests
{
public class TestConfiguration : IConfiguration
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can move this into Elastic.Apm.Tests.Utilities - as long as we only use it here, it's totally fine at this place as well...

Copy link
Contributor

Choose a reason for hiding this comment

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

I think keeping it here for now is reasonable, and look to refactor MockConfiguration to replace this

@russcam russcam merged commit e953480 into master Nov 9, 2021
@russcam russcam deleted the update-spec-files-20211012142323 branch November 9, 2021 02:14
@russcam russcam added v1.12.0 enhancement New feature or request labels Nov 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[META 527] User-agent header for transport
4 participants