Resolve schemaUrl when merging ResourceInfo by lalex · Pull Request #627 · open-telemetry/opentelemetry-php

lalex · 2022-03-21T19:12:25Z

Continue with the #445. This PR to resolve a resulting schemaUrl when merging ResourceInfos. Reference to the spec.

codecov · 2022-03-21T19:14:16Z

Codecov Report

Merging #627 (ea468b3) into main (0bc3731) will decrease coverage by 0.18%.
The diff coverage is 0.00%.

❗ Current head ea468b3 differs from pull request most recent head 89d4d43. Consider uploading reports for the commit 89d4d43 to get more accurate results

@@             Coverage Diff              @@
##               main     #627      +/-   ##
============================================
- Coverage     84.45%   84.27%   -0.19%     
- Complexity     1141     1146       +5     
============================================
  Files           127      127              
  Lines          2766     2772       +6     
============================================
  Hits           2336     2336              
- Misses          430      436       +6

Flag	Coverage Δ
7.4	`84.21% <0.00%> (-0.19%)`	⬇️
8.0	`84.27% <0.00%> (-0.19%)`	⬇️
8.1	`84.27% <0.00%> (-0.19%)`	⬇️

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

Impacted Files	Coverage Δ
src/SDK/Resource/ResourceInfoFactory.php	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bc3731...89d4d43. Read the comment docs.

tidal · 2022-03-21T21:46:17Z

Thank you!

tidal · 2022-03-21T21:27:43Z

@@ -42,15 +45,15 @@ public static function create(AttributesInterface $attributes, ?string $schemaUr
     */
    public static function merge(ResourceInfo ...$resources): self


Please note that this method is about to be extracted out.

tidal · 2022-03-21T21:31:43Z

+        $initialResource = array_shift($resources);
+
+        return array_reduce($resources, function (ResourceInfo $old, ResourceInfo $updating): ResourceInfo {
+            return $old->mergeWith($updating);


Calling the mergeWith will result in unnecessary object creation when used in defaultResource, however this could be optomized later.

tidal · 2022-03-21T21:33:04Z

        return $this->schemaUrl;
    }
+
+    private function mergeWith(ResourceInfo $resource): ResourceInfo


Please make this method public and add it to the interface, since static mergeis subject to be moved.

tidal · 2022-03-21T21:35:13Z

+        $attributes = $this->attributes->toArray() + $resource->getAttributes()->toArray();
+
+        $schemaUrl = null;
+        if (null === $this->schemaUrl) {


Please, do not use yoda style.
Please, use the accessor for this property here, since the accessor is used in the updatig resource and this will make it easier to read.

I'm not sure that php-cs-fixer is configured properly to avoid yoda style.

No it's not, as there is a lot of it in the code atm, which would needed to be changed (not sure if the fixer can handle this correctly).
However we have agreed in a meetig, that we'd rather want to avoid it in the future.

tidal · 2022-03-21T21:36:07Z

+        $schemaUrl = null;
+        if (null === $this->schemaUrl) {
+            $schemaUrl = $resource->getSchemaUrl();
+        } elseif (null === $resource->getSchemaUrl()) {


Please, do not use yoda style.

tidal · 2022-03-21T21:39:09Z

+            $schemaUrl = $this->schemaUrl;
+        } elseif ($this->schemaUrl === $resource->getSchemaUrl()) {
+            $schemaUrl = $this->schemaUrl;
+        } else {


Please refactor these conditions in a method which simply resolves the schemaURL and remove else/elseif.

This piece of code is self-describing and follows the specification. A simplified version could hide an original logic.

I wouldn't go for a simplified version, but rather avoiding the "IFs" with return statements.
I know it's to be found like that in the spec, which doesn't really make the spec better in my eyes.
However avoiding "IFs" is my personal issue, so I'd at least ask for it. Feel free to ignore my request. :)

tidal · 2022-03-21T21:40:23Z

+        } elseif (null === $resource->getSchemaUrl()) {
+            $schemaUrl = $this->schemaUrl;
+        } elseif ($this->schemaUrl === $resource->getSchemaUrl()) {
+            $schemaUrl = $this->schemaUrl;


tidal · 2022-03-21T21:43:19Z

+     */
+    public function test_merge_schema_url($oldScheme, $updatingScheme, $expectedScheme, $shouldLog): void
+    {
+        if ($shouldLog) {


Please, do not use conditions in tests. This can simply be a separate test.

Good point, thanks!

tidal · 2022-03-21T21:44:56Z

+
+        $primary = ResourceInfo::create(new Attributes([]), $oldScheme);
+        $secondary = ResourceInfo::create(new Attributes([]), $updatingScheme);
+        $result = ResourceInfo::merge($primary, $secondary);


Please test against the mergeWithmethod here. (See above)

lalex · 2022-03-22T11:28:11Z

Let's postpone this PR until the ResourceInfo refactoring is finished in #621.

tidal · 2022-03-22T11:51:50Z

Let's postpone this PR until the ResourceInfo refactoring is finished in #621.

Ok.

kishannsangani · 2022-03-25T08:13:08Z

Let's postpone this PR until the ResourceInfo refactoring is finished in #621.

@lalex ResourceInfo refactoring is done in #628 and has been merged in the main branch.

bobstrecansky · 2022-03-30T17:40:51Z

I think you should be able to continue work on this when you'd like to @lalex !

lalex · 2022-04-01T16:11:49Z

I've decided to not to go too deep into the process of schema merging. The only significant thing is done is the setting an empty schemaUrl in case of conflict detected.
I think the PR is ready to be merged.

PS. I didn't find a proper way to write a log message from static context. @tidal, could you get a hint?

brettmc · 2022-04-01T23:19:31Z

PS. I didn't find a proper way to write a log message from static context.

Hi @lalex - I don't think logging messages from static methods has come up yet, but looking at LogsMessagesTrait, I suspect it can be changed to use static functions so that it works for both static and non-static use cases. I will have time to play around with that later today.

tidal · 2022-04-02T05:01:38Z

@lalex Thanks for your work!

PS. I didn't find a proper way to write a log message from static context. @tidal, could you get a hint?

You could access the LoggerHolder's get method to get the logger as it's static anyway
(The constructor is just there to help with dependendy injection in frameworks)

Anyway,I will merge the PR for now.

lalex requested review from bobstrecansky, brettmc, tidal and zsistla as code owners March 21, 2022 19:12

tidal suggested changes Mar 21, 2022

View reviewed changes

Extract a Resource schemaUrl merging method

89d4d43

lalex force-pushed the schema_url branch from b995846 to 89d4d43 Compare April 1, 2022 15:41

tidal approved these changes Apr 2, 2022

View reviewed changes

tidal merged commit 445e932 into open-telemetry:main Apr 2, 2022

yuktea mentioned this pull request Jun 4, 2022

Support schemaUrl #445

Closed

		@@ -42,15 +45,15 @@ public static function create(AttributesInterface $attributes, ?string $schemaUr
		*/
		public static function merge(ResourceInfo ...$resources): self

Uh oh!

Conversation

lalex commented Mar 21, 2022

Uh oh!

codecov Bot commented Mar 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tidal commented Mar 21, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lalex commented Mar 22, 2022

Uh oh!

tidal commented Mar 22, 2022

Uh oh!

kishannsangani commented Mar 25, 2022

Uh oh!

bobstrecansky commented Mar 30, 2022

Uh oh!

lalex commented Apr 1, 2022

Uh oh!

brettmc commented Apr 1, 2022

Uh oh!

tidal commented Apr 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Mar 21, 2022 •

edited

Loading