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

confgenerator: add resource_name label using modify fields. #544

Merged

Conversation

franciscovalentecastro
Copy link
Contributor

@franciscovalentecastro franciscovalentecastro commented Apr 20, 2022

b/169784211

@franciscovalentecastro franciscovalentecastro changed the title add resource_name label using modify fields. confgenerator: add resource_name label using modify fields. Apr 20, 2022
confgenerator/logging.go Outdated Show resolved Hide resolved
confgenerator/logging.go Outdated Show resolved Hide resolved
@@ -209,7 +209,7 @@ func (l *Logging) generateFluentbitComponents(userAgent string, hostInfo *host.I
}
components = append(components, processor.Components(tag, strconv.Itoa(i))...)
}
components = append(components, setLogNameComponents(tag, rID, receiver.Type())...)
components = append(components, setLogNameComponents(tag, rID, receiver.Type(), hostInfo.Hostname)...)
Copy link
Member

Choose a reason for hiding this comment

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

hostname isn't necessarily the same as the instance name - the resource name has to come from the GCE metadata server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went back to explore the initial discussion (b/169784211#comment18) with @igorpeshansky and @qingling128 about this task and realized why it was ok to use hostInfo.Hostname.

The value that is used in google-fluentd (which this feature wants to mirror) is actually the Socket.gethostname which is equivalent of getting os.Hostname in go with gopsutil.

What do you think @quentinmit ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for checking! Yeah, having the same behavior as Fluentd is sufficient for now.

@franciscovalentecastro franciscovalentecastro added the kokoro:force-run Forces kokoro to run integration tests on a CL label Apr 25, 2022
@kokoro-team kokoro-team removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Apr 25, 2022
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-add-resource-name-modify-fields branch 3 times, most recently from fabefab to 341bf44 Compare April 28, 2022 19:12
@franciscovalentecastro franciscovalentecastro added the kokoro:force-run Forces kokoro to run integration tests on a CL label Apr 28, 2022
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Apr 28, 2022
@franciscovalentecastro franciscovalentecastro added kokoro:force-run Forces kokoro to run integration tests on a CL and removed kokoro:force-run Forces kokoro to run integration tests on a CL labels Apr 28, 2022
@stackdriver-instrumentation-release stackdriver-instrumentation-release removed the kokoro:force-run Forces kokoro to run integration tests on a CL label Apr 28, 2022
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-add-resource-name-modify-fields branch from e0e2825 to 260735b Compare April 29, 2022 20:38
@franciscovalentecastro franciscovalentecastro marked this pull request as ready for review April 29, 2022 20:39
Copy link
Contributor

@qingling128 qingling128 left a comment

Choose a reason for hiding this comment

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

LGTM

integration_test/ops_agent_test.go Outdated Show resolved Hide resolved
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-add-resource-name-modify-fields branch from 149aac2 to 485c65f Compare May 2, 2022 15:14
@franciscovalentecastro franciscovalentecastro force-pushed the fcovalente-add-resource-name-modify-fields branch from f9c44ed to 93021a6 Compare May 2, 2022 16:38
@franciscovalentecastro franciscovalentecastro merged commit b653dab into master May 2, 2022
@franciscovalentecastro franciscovalentecastro deleted the fcovalente-add-resource-name-modify-fields branch May 2, 2022 17:49
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.

5 participants