Skip to content

Fix entity naming mechanism failure in MAL#7790

Merged
wu-sheng merged 3 commits into
masterfrom
fix-meter-name
Sep 24, 2021
Merged

Fix entity naming mechanism failure in MAL#7790
wu-sheng merged 3 commits into
masterfrom
fix-meter-name

Conversation

@wu-sheng
Copy link
Copy Markdown
Member

  • If this pull request closes/resolves/fixes an existing issue, replace the issue number. Closes #.
  • Update the CHANGES log.

Fix entity(service/instance/endpoint) names in the MAL system(prometheus, native meter, open census, envoy metric service) are not controlled by core's naming-control mechanism.

@wu-sheng wu-sheng added bug Something isn't working and you are sure it's a bug! core feature Core and important feature. Sometimes, break backwards compatibility. backend OAP backend related. labels Sep 24, 2021
@wu-sheng wu-sheng added this to the 8.8.0 milestone Sep 24, 2021
hanahmily
hanahmily previously approved these changes Sep 24, 2021
Copy link
Copy Markdown
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

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

This fix is fragile, MeterEntity exposes many means to construct its instances, this patch only covers the static methods newService, newServiceInstance, newEndpoint, newServiceRelation, future developers who use the uncovered cases to construct the entity might cause the bug again.

The other uncovered cases:

  1. MeterEntity.builder().serviceName("").instanceName("")..., if it's designed to only use the newXxxx methods to construct MeterEntity, we should update the @Builder annotation to @Builder(toBuilder = true, access = AccessLevel.PRIVATE)
  2. meterEntity.toBuilder() returns another builder, which is the same case with (1.), we'd better also remove toBuilder = true in @Builder
  3. new constructor (although it's package-private), we'd better also make it private.

@wu-sheng
Copy link
Copy Markdown
Member Author

@kezhenxu94 I removed @Builder and fixed the tests.

@wu-sheng wu-sheng merged commit b7ba282 into master Sep 24, 2021
@wu-sheng wu-sheng deleted the fix-meter-name branch September 24, 2021 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend OAP backend related. bug Something isn't working and you are sure it's a bug! core feature Core and important feature. Sometimes, break backwards compatibility.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants