Remove service instances, add labels to service #83
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After much discussion, we decided that we will keep global labels as they are for now, and eventually remove them when we have a replacement. The replacement is expected to be something along the lines of more explicit, user-configurable dimensions. Between now and then we may add more fields to the service aggregation key for common use cases, e.g. service namespace. The service instances concept does not align with that future, so we're reverting to aggregating by global labels at the service level. The implication is that global labels are expected to have low cardinality, and if they do not then users can expect a service with high cardinality global labels to interfere with the collection of metrics for other services (i.e. causing them to overflow.)
axw
force-pushed
the
rm-service-instance
branch
from
3ba56c1 to
50d683c
Compare
kruskall
approved these changes
Aug 14, 2023
| @@ -122,6 +122,7 @@ func (k *serviceAggregationKey) ToProto() *aggregationpb.ServiceAggregationKey { | |||
| pb.ServiceEnvironment = k.ServiceEnvironment | |||
| pb.ServiceLanguageName = k.ServiceLanguageName | |||
| pb.AgentName = k.AgentName | |||
| pb.GlobalLabelsStr = []byte(k.GlobalLabelsStr) | |||
There was a problem hiding this comment.
I wonder if this and the below k.GlobalLabelsStr = string(pb.GlobalLabelsStr) is continuously creating copies and we should store everything as strings or find an alternative.
Either way, this is not changing the previous behaviour so approving!
There was a problem hiding this comment.
It has to be a []byte because the contents may not be valid UTF-8, which is required for protobuf strings.
There was a problem hiding this comment.
String must always contain UTF-8 as per proto requirements. Since we encode the global labels as proto I don't think it will hold true.
There was a problem hiding this comment.
Oops, missed axw's message 😅
lahsivjar
approved these changes
Aug 14, 2023
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
After much discussion, we decided that we will keep global labels as they are for now, and eventually remove them when we have a replacement. The replacement is expected to be something along the lines of more explicit, user-configurable dimensions. Between now and then we may add more fields to the service aggregation key for common use cases, e.g. service namespace.
The service instances concept does not align with that future, so we're reverting to aggregating by global labels at the service level. The implication is that global labels are expected to have low cardinality, and if they do not then users can expect a service with high cardinality global labels to interfere with the collection of metrics for other services (i.e. causing them to overflow.)