Skip to content

Conversation

@mpoc
Copy link
Contributor

@mpoc mpoc commented Mar 4, 2024

In some cases, the PowerShell command that is used to detect the hostname will contain extra output before the hostname, e.g. the Microsoft copyright banner or any output that is printed in the PowerShell profiles. This causes invalid hostnames to be sent to the APM server.

This change ensures that the PowerShell output is limited only to the hostname.

@trentm
Copy link
Member

trentm commented Mar 4, 2024

@mpoc Thanks!

In some cases,

Do you happen to know what cases? For example, has the default behaviour of powershell.exe on whether to show a logo changed over time? Or perhaps it depends on some system configuration?

Anyway, these changes look good to me.

(Note to self: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_powershell_exe?view=powershell-5.1 for notes on the powershell CLI options.)

@trentm trentm self-assigned this Mar 4, 2024
@mpoc
Copy link
Contributor Author

mpoc commented Mar 4, 2024

Not sure, I couldn't find a definitive answer.

I just wanted to clarify that I wasn't suggesting the banner shows up inconsistently. What I meant was that log messages in profile scripts will break the hostname detection. Likewise, anything printed before the hostname, like the logo banner, will break it for the same reason.

@trentm
Copy link
Member

trentm commented Mar 4, 2024

run docs-build

@trentm
Copy link
Member

trentm commented Mar 4, 2024

I just wanted to clarify [...]

Understood. Thanks.

@trentm
Copy link
Member

trentm commented Mar 5, 2024

This passes "test-windows" before and after. And our tests do include a test of the "system.detected_hostname":

const system = Object.assign({}, payload.system);
t.ok(
system.detected_hostname.startsWith(os.hostname().toLowerCase()),
'metadata: system.detected_hostname',
);
delete system.detected_hostname;

So I think this is good.

I'll follow up with a PR on our specs for this:
https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname

@trentm trentm merged commit e33148f into elastic:main Mar 5, 2024
trentm added a commit to elastic/apm that referenced this pull request Mar 5, 2024
…d logo that could break detection

If a Windows system has a powershell profile that emits output, then it
could pollute the output used to read the hostname value. Likewise with
the powershell logo output, though I don't know if/when that logo
appears -- it does not in apm-agent-nodejs' CI tests.

Refs: elastic/apm-agent-nodejs#3899
@trentm
Copy link
Member

trentm commented Mar 5, 2024

I'll follow up with a PR on our specs for this:
https://github.com/elastic/apm/blob/main/specs/agents/metadata.md#hostname

elastic/apm#851 for that.

trentm added a commit to elastic/apm that referenced this pull request Mar 12, 2024
…d logo that could break detection (#851)

If a Windows system has a powershell profile that emits output, then it
could pollute the output used to read the hostname value. Likewise with
the powershell logo output, though I don't know if/when that logo
appears -- it does not in apm-agent-nodejs' CI tests.

Refs: elastic/apm-agent-nodejs#3899
PeterEinberger pushed a commit to fpm-git/apm-agent-nodejs that referenced this pull request Aug 20, 2024
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.

2 participants