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

inits Deployment contrib #2498

Merged
merged 20 commits into from
May 10, 2023
Merged

inits Deployment contrib #2498

merged 20 commits into from
May 10, 2023

Conversation

mhausenblas
Copy link
Member

@mhausenblas mhausenblas commented Mar 13, 2023

@mhausenblas mhausenblas requested review from a team and Aneurysm9 and removed request for a team March 13, 2023 18:29
Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Reposting #2312 (comment)

Are we introducing new deployment concepts or replacing Agent/Gateway with Decentralized/Centralized as 1:1? It doesn't seem to be 1:1 based on the docs:

  • Agent is supposed to represent an installation on one host so that instrumentation libraries can point to local endpoints like http://localhost:4318. The decentralized doc says collector.example.com:4318 instead. Also, the decentralized section mentions "Clear 1:1 mapping between application and collector" in "Pros" section, which is right for the agent term as well, but it confuses me when I read the first paragraph that seems to contradict:

The decentralized collector deployment pattern consists of applications—instrumented with an OpenTelemetry SDK using OpenTelemetry protocol (OTLP)—or other collectors (using the OTLP exporter) that send telemetry signals to one or more collectors.

  • Several Collector pull-based receivers are intended to run in the agent (decentralized?) mode, for example, hostmetrics receiver. If we are extending the documentation, I believe it's worth mentioning.

In general, I don't fully agree that decentralized/centralized terms are easier to understand than agent/gateway. I added this topic to the next Collector SIG meeting's agenda https://docs.google.com/document/d/1r2JC5MB7GupCE7N32EwGEXs9V_YIsPgoFiLP4VWVMkE/edit

@smithclay
Copy link
Contributor

smithclay commented Mar 14, 2023

@dmitryax Whatever term is used, we should add/link/update the glossary and include the other terminology that is used to refer to the pattern.

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I love this content. I also vote not to change to Centralized and Decentralized. Personally I am good with Agent and Gateway, although during this SIG today it sounds like some would opt to change only Gateway to something else.

@mhausenblas
Copy link
Member Author

Thanks for the feedback @TylerHelmuth and I will implement whatever the consensus is. It sounds to me there is no clear preference yet either way?

@cartermp
Copy link
Contributor

I would be in favor of merging this as-is if there's no strong consensus. We can update it later when there is consensus. It's far more valuable to have these docs in now than to hold it up over two words :)

@TylerHelmuth
Copy link
Member

It sounds to me there is no clear preference yet either way?

There was definitely consensus on Agent. @jpkrohling @dmitryax dissented on Gateway.

Seeing as Gateway is currently the term used on the site and seems to be the most prevalent term, if there is pressure to get this doc through soon, I would say using Agent and Gateway at the moment is proper. In the future, if Collector maintainers want to switch from Gateway to something else thats fine, but at least for this PR we would not be introducing any new terms. I would like to avoid switching from Agent and Gateway to Centralized and Decentralized only to switch again to Agent and some new term. Better in my mind to keep Agent and Gateway for now and switch Gateway later.

As always, nothing is every as exciting and as engaging as naming stuff lol

@open-telemetry/collector-approvers @open-telemetry/collector-contrib-approvers

@mhausenblas
Copy link
Member Author

Thanks for the clarification @TylerHelmuth

In the future, if Collector maintainers want to switch from Gateway to something else thats fine

Are you suggesting that the collector maintainers are the only ones who can and should decide terminology?

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Are you suggesting that the collector maintainers are the only ones who can and should decide terminology?

I don't think maintainers should be the only ones who decide on terminology. I do think it's important that new terminology be introduced carefully, especially when there is already terminology defined to address those concepts.

I agree that this documentation is a significant improvement, and I'd prefer not to get stuck in naming limbo. Maybe a vote on this PR for each term would allow us to move forward with it?

content/en/docs/collector/deployment/_index.md Outdated Show resolved Hide resolved
content/en/docs/collector/deployment/best-practices.md Outdated Show resolved Hide resolved
content/en/docs/collector/deployment/best-practices.md Outdated Show resolved Hide resolved
content/en/docs/collector/deployment/best-practices.md Outdated Show resolved Hide resolved
@mhausenblas
Copy link
Member Author

Thanks for your feedback @codeboten!

Concerning:

I do think it's important that new terminology be introduced carefully, especially when there is already terminology defined to address those concepts.

Just to clarify, it's not that I'm pulling things out of the thin air, right? :)
I was following @jpkrohling input and suggestion from #1368 (comment). I just mention that here to make it clear: I don't have a dog in this race, I just want to make sure that we ship stuff, ASAP.

I agree that this documentation is a significant improvement, and I'd prefer not to get stuck in naming limbo.

+1

Maybe a vote on this PR for each term would allow us to move forward with it?

Might not be necessary. Unless I hear a good argument why we should introduce the new terminology from supportive folks such as @jpkrohling I will undo the changes and we keep the old terminology. Seems OpenTelemetry is getting slowly big enough for these kind of coordination issues … we really need a SIG Release like Kubernetes has ha ha

@codeboten
Copy link
Contributor

I just mention that here to make it clear: I don't have a dog in this race, I just want to make sure that we ship stuff, ASAP

100%. My only concern is around not creating confusion in the process :D When this PR was discussed in the collector SIG yesterday there was a lot of confusion in the discussion :)

Seems OpenTelemetry is getting slowly big enough for these kind of coordination issues

Yes... I'd say at least for terminology, maybe the coordination point could be the glossary in the specification? Which I sadly just realized differs from the glossary in the documentation :(

@dmitryax
Copy link
Member

@jpkrohling mentioned during the SIG that he didn't suggest Centralized/Decentralized. He doesn't agree with the Gateway term. I am up for improving the documentation but I don't think we should introduce new concepts that collector maintainers don't agree with. The new concepts should be used in the codebase, not only in the docs.

I agree with @TylerHelmuth. I think we can improve the documentation keeping the existing terminology, and decide on a Gateway replacement as a separate effort.

@cartermp
Copy link
Contributor

Let's say we just change the names in this PR to Agent and Gateway (even if we're not 100% happy with that existing term) - is that sufficient to get this merged?

@TylerHelmuth
Copy link
Member

Let's say we just change the names in this PR to Agent and Gateway (even if we're not 100% happy with that existing term) - is that sufficient to get this merged?

Yes, this is what I was proposing. 👍 from me.

@cartermp
Copy link
Contributor

Alrighty, let's consider that the plan for now. @mhausenblas could you make the terminology change? Then I think this is pretty close to getting merged. Collector folks can orthogonally decide on what the "right" terminology is and the content can be updated accordingly. But for now, there's too much good stuff in here already to let it go unmerged 🙂

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

Hi. There should be no change to themes/docsy in this PR. You probably need to rebase then rebuild to fully sync with the latest Docsy.

@mhausenblas
Copy link
Member Author

Thanks for all the feedback, will address and wrap up on Mon Mar 20 (today's St Patrick's day here in Ireland and … well ;)

You probably need to rebase then rebuild to fully sync with the latest Docsy.

@chalin before I f0rk up again, what exactly does "rebuild" mean? Can I ping you on Monday?

@chalin
Copy link
Contributor

chalin commented Mar 17, 2023

@chalin before I f0rk up again, what exactly does "rebuild" mean? Can I ping you on Monday?

@mhausenblas - yes certainly, though I can clarify here (sorry, could have been clearer the first time 🤷🏼‍♂️): generally after a rebase, git submodules might not be in sync. By "rebuild" I meant re-run npm run build, which in turn runs npm run get:submodule to sync to submodules. I hope that helps, if not DM me (though I'll probably be OOO part of next week).

🍀🍀 Happy St. Paddy's Day! 🍀🍀

@svrnm
Copy link
Member

svrnm commented Mar 22, 2023

Just looking through that PR. I would love to have that in as soon as possible, I am just wondering if we can find a way to simplify the PR and have some follow-up PRs later down to get it closed a little bit quicker.

Just a few suggestions:

  • Pros & Cons go into their own PR
  • Best Practices go into their own PR

WDYT?

@chalin
Copy link
Contributor

chalin commented Mar 29, 2023

@mhausenblas - if you prefer, I can fix the Docsy issue. Let me know.

@mhausenblas
Copy link
Member Author

@chalin thanks, that would be lovely! I plan to wrap up tomorrow …

@chalin
Copy link
Contributor

chalin commented May 10, 2023

Rebased and checks are passing (even though I can see only one of the following two checks in the "checks summary" above):

image

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I'm fine merging this as is if you promise to expand on the cons for the agent pattern as part of a follow-up PR :-)

content/en/docs/collector/deployment/agent.md Outdated Show resolved Hide resolved
Cons:

- Scalability (human and load-wise)
- Inflexible
Copy link
Member

Choose a reason for hiding this comment

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

Why? Reading this myself, I can't tell exactly what you had in mind.


Cons:

- Scalability (human and load-wise)
Copy link
Member

Choose a reason for hiding this comment

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

Why? What are the gotchas regarding scalability on this pattern? You could say that sidecars could become an unnecessary overhead and that daemon sets are harder to scale than deployments. You need to expand a bit instead of just stating "Scalability" :-)

@mhausenblas
Copy link
Member Author

@jpkrohling deal! Added the ask to my list of things that need to be addressed for the follow-up PR.

@cartermp cartermp dismissed dmitryax’s stale review May 10, 2023 18:24

Other reviews supersede, changes addressed

@cartermp
Copy link
Contributor

And we're doin' it.

@cartermp cartermp merged commit d98c705 into open-telemetry:main May 10, 2023
@cartermp
Copy link
Contributor

@mhausenblas can you file an issue on the repo here with a link to @jpkrohling's feedback to address as a follow-up?

@mhausenblas
Copy link
Member Author

Thanks @cartermp and see #2692

@mhausenblas mhausenblas deleted the col-deploy branch May 11, 2023 07:45
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.

10 participants