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

add user-agent spec + gherkin spec #514

Merged
merged 5 commits into from
Oct 12, 2021

Conversation

SylvainJuge
Copy link
Member

@SylvainJuge SylvainJuge commented Sep 27, 2021

  • Adding a shared spec for HTTP user-agent header value
  • Adding gherkin executable spec (might be overkill)

Fixes #509

@apmmachine
Copy link

apmmachine commented Sep 27, 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-10-11T14:55:19.200+0000

  • Duration: 6 min 7 sec

  • Commit: 0e2bc44

Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Thanks!

specs/agents/transport.md Show resolved Hide resolved
tests/agents/gherkin-specs/user_agent.feature Outdated Show resolved Hide resolved
specs/agents/transport.md Outdated Show resolved Hide resolved
@bryce-b
Copy link
Contributor

bryce-b commented Sep 29, 2021

since the iOS agent routes through open-telemetry, will that need to be taken into account? I'm pretty sure there is are relevant Otel headers that are transmitted, but they may need to be normalized to a common, Elastic compatible format. It could be valuable to a generalized solution for all Otel sources in that regard, but it's totally possible for iOS to conform to this spec as well.

Co-authored-by: Trent Mick <trentm@gmail.com>
@SylvainJuge
Copy link
Member Author

since the iOS agent routes through open-telemetry, will that need to be taken into account? I'm pretty sure there is are relevant Otel headers that are transmitted, but they may need to be normalized to a common, Elastic compatible format. It could be valuable to a generalized solution for all Otel sources in that regard, but it's totally possible for iOS to conform to this spec as well.

I haven't found anything in the OTLP specification about this. I think that as we currently use HTTP to communicate with apm-server it would be relevant for the iOS agent to conform to this to help debugging, especially when devices are in the wild it would be convenient to gather statistics about agent versions and/or which services from server HTTP access logs.

@Mpdreamz
Copy link
Member

Mpdreamz commented Oct 4, 2021

Have we considered aligning this spec with the clients specification for user-agent? I think the only change needed would be

- {elasticapm-${language}/${agent.version}
+ {repository-name}/${agent.version}

And inclusion of service name version in brackets

- elasticapm-python/6.4.0 myservice/v42.7
+ apm-agent-python/6.4.0 (myservice v42.7)

https://github.com/elastic/clients-team/blob/master/knowledgebase/specifications/030-user-agent.md

Out of scope but raising for awareness: The clients also expose a 2nd headerfor more metadata that Cloud understands and generates reports over:

https://github.com/elastic/clients-team/blob/master/knowledgebase/specifications/client-meta-header.md

We might want to align the constraints on version numbers from that spec too

@SylvainJuge
Copy link
Member Author

That's a very good idea @Mpdreamz ! I'll update the current spec to match this.

Thanks for providing a link to the spec in clients-team repo too (not public unfortunately), we could also use that as a later improvement (or to provide what is being removed for the Node.js agent).

@trentm
Copy link
Member

trentm commented Oct 29, 2021

If it helps others, here is the Node.js implementation of this: elastic/apm-agent-nodejs@052e396

Mostly of possible interest is the sanitization of serviceName and serviceVersion before including in the User-Agent string via:

const commentBadChar = /[^\t \x21-\x27\x2a-\x5b\x5d-\x7e\x80-\xff]/g
const sanitizedServiceVersion = conf.serviceVersion.replace(commentBadChar, '_')
...

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.

Add service name and version to User-Agent header
8 participants