Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support monolithic deployment mode #722
Support monolithic deployment mode #722
Changes from 22 commits
edb2a72
51a2b26
41dbed1
8aaa8b5
442004c
7d3ce55
84cba0c
cd797bc
355a79e
82fef56
0f490dd
c84f6bd
bd54f6b
8c66093
5874547
3b86d3a
3faae98
a027746
7af52c4
19d6e07
f29369f
f0c9ffc
c0343f1
0c9b6e6
fd37ff8
9acad7b
754486e
3f57be4
852d0e2
19ca826
cd64880
a9e41e9
6657c89
9a95fce
1cf6976
16b7030
822c016
bf76bef
4c6715a
c5eba0d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Asking again, can we reuse some of the structs from the microservies type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, question is do we value consistency inside the same CR or consistency between the two CRs more?
vs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first example is consistent with the rest of the Monolithic CR, and allows additional settings for prometheusRules or serviceMonitors in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer consistency on the same CRD, just one question, in the future is possible to have some sort of consolidation in order to get both? That will imply a breaking change though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of maybe doing this change in v1alpha2 of TempoStack if we have a consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for not reusing the same as microservices? The only reason I think is because the microservices could include other configs in the future. If that is the reason I'm ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we worked on the same feature at the same time. I'll check if I can reuse the struct and logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the PR and reused the struct and logic from the TempoStack now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One question, Why this have it's own spec and inside only have one structure? or why to not use
MonolithicTracesStorageSpec
directly. Is this for mimic the tempo configuration?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's to mimic the tempo configuration. I thought maybe tempo has plans to store other things in the future, so I'll keep the same here also.
But I don't have very strong opinions on this, I could remove that extra layer if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be the only protocol supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to make the gateway a drop-in feature, so the service ports should not change if gateway is enabled or not. So I can only support protocols which the gateway also supports.
Afaics the gateway only supports otlp/grpc and otlp/http, right?
With the general move to the OTEL SDK, and using the OTEL collector, I think it's fine to only support OTLP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be the ingress spec reused from the microservices? There are more settings users might want to configure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Already ready in #755:
vs spec of TempoStack:
(ingressClassName should have been under ingress, as it doesn't apply to route)
Do we value we value consistency inside the same CR or consistency between the two CRs more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment, I'd prefer consistency on the same CR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we reuse smth from the microservice type?
e.g. the whole observability spec
tempo-operator/apis/tempo/v1alpha1/tempostack_types.go
Line 141 in cbe369b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be inconsistent with the
style of the CR. I'd prefer to migrate the
TempoStack
, maybe in the next CRD version? Shouldn't be too difficult to create a conversion webhook for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so we agreed on using the enabled field which is better supported in tools like
kustomize
andkubectl edit
(the empty structs are removed). Are we going to reuse some parts from the monolithic APIs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can reuse a TLS struct, the multitenancy structs, ManagementStateType, LimitSpec and the storage secret.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this rendered in the logs? Isn't it too long?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll print this line:
if debug logs are enabled (
go run ./main.go --zap-log-level=debug start
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But as we'll set the defaults in the reconcile loop now, I'm removing this log statement.
I'll keep it there for the validating webhook, which is still in use.