Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Added -unknown suffix to container name #288

Open
wants to merge 2 commits into
base: lyft
Choose a base branch
from

Conversation

jcarey03
Copy link

@jcarey03 jcarey03 commented Oct 5, 2022

Signed-off-by: Jason Carey jcarey@lyft.com

TL;DR

Adding -unknown suffix to container name for Lyft logging agent to detect and send logs to logging pipeline.

Type

  • Bug Fix
  • Feature
  • Plugin

Signed-off-by: Jason Carey <jcarey@lyft.com>
Signed-off-by: Jason Carey <jcarey@lyft.com>
@hamersaw
Copy link
Contributor

hamersaw commented Oct 7, 2022

@jcarey03 can you explain a little bit more about the use-case here? I'm a little concerned about modifying the container name on all deployments. How is the "-unknown" detected by the agent? Is there a different field that can be set to identify these containers?

@jcarey03
Copy link
Author

jcarey03 commented Oct 10, 2022

@hamersaw This PR is only scoped for the flyteorg:lyft branch as it pertains to Lyft-specific infrastructure; it will not be pulled into the master branch. With that said, I'm pretty sure there is a much better way to make this feature configurable if that is a desirable feature for the Flyte community.

Our logging infrastructure uses fluentbit as a log collector and forwarder and is configured to monitor containers with a specific suffix. We decided to make the basic change first and test integration. Once that is confirmed, we'll circle back and improve upon the naive hardcoded solution to something more configurable, but again, the thinking is that this is only for Lyft and not general Flyte.

@codecov
Copy link

codecov bot commented Oct 10, 2022

Codecov Report

❗ No coverage uploaded for pull request base (lyft@ffd74b6). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             lyft     #288   +/-   ##
=======================================
  Coverage        ?   61.40%           
=======================================
  Files           ?      139           
  Lines           ?     8594           
  Branches        ?        0           
=======================================
  Hits            ?     5277           
  Misses          ?     2831           
  Partials        ?      486           
Flag Coverage Δ
unittests 61.03% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@hamersaw
Copy link
Contributor

@hamersaw This PR is only scoped for the flyteorg:lyft branch as it pertains to Lyft-specific infrastructure; it will not be pulled into the master branch. With that said, I'm pretty sure there is a much better way to make this feature configurable if that is a desirable feature for the Flyte community.

Oh sorry! did not catch this PR was into the Lyft branch.

Our logging infrastructure uses fluentbit as a log collector and forwarder and is configured to monitor containers with a specific suffix. We decided to make the basic change first and test integration. Once that is confirmed, we'll circle back and improve upon the naive hardcoded solution to something more configurable, but again, the thinking is that this is only for Lyft and not general Flyte.

So, for what my two cents it worth here, there was recently a community contribution to enable default configuration of containers using the default PodTemplate work. I know their use-case was for logging in the primary container, but I can't recall what fields they were using to identify the container or link up the logging framework. Not sure if this is worth integrating into for your setup, but may be worth a quick look!

@jcarey03
Copy link
Author

Thanks for sharing. We'll take a look.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants