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

[OneCollectorExporter] Extension feature improvements #1327

Conversation

CodeBlanch
Copy link
Member

@CodeBlanch CodeBlanch commented Aug 25, 2023

Changes

  • Extends the common schema extension feature to allow consumers to own entire elements
  • Adds the utf8 encoded json to what is stored in the extension cache
  • Adds a special case for IReadOnlyList<KeyValuePair<string, object?>> values in json serializer

TODOs

  • Appropriate CHANGELOG.md updated for non-trivial changes

@CodeBlanch CodeBlanch requested a review from a team August 25, 2023 21:13
@github-actions github-actions bot requested a review from reyang August 25, 2023 21:13
@utpilla utpilla added the comp:exporter.onecollector Things related to OpenTelemetry.Exporter.OneCollector label Aug 25, 2023
@codecov
Copy link

codecov bot commented Aug 25, 2023

Codecov Report

Merging #1327 (cf340f1) into main (ebc40a0) will increase coverage by 0.23%.
Report is 1 commits behind head on main.
The diff coverage is 97.79%.

❗ Current head cf340f1 differs from pull request most recent head 0528ea9. Consider uploading reports for the commit 0528ea9 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1327      +/-   ##
==========================================
+ Coverage   74.50%   74.73%   +0.23%     
==========================================
  Files         266      267       +1     
  Lines        9657     9757     +100     
==========================================
+ Hits         7195     7292      +97     
- Misses       2462     2465       +3     
Files Changed Coverage
...erialization/CommonSchemaJsonSerializationState.cs 97.43%
...ector/Internal/ExtensionFieldInformationManager.cs 97.70%
...OneCollector/Internal/ExtensionFieldInformation.cs 100.00%
...rialization/CommonSchemaJsonSerializationHelper.cs 100.00%

📢 Thoughts on this report? Let us know!.


var state = new CommonSchemaJsonSerializationState("Test", writer);

state.AddExtensionAttribute(new KeyValuePair<string, object?>(
Copy link
Member

Choose a reason for hiding this comment

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

Consider having another test case:

var metadata = new Dictionary<string, object>
               {
                   ["f"] = new Dictionary<string, object>
                   {
                       ["Id"] = 1234,
                   },
               };
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.metadata", metadata);

metadata["x"] = 1; // modify the object after AddExtensionAttribute

// ...

Assert.Equal(
            "{\"ext\":{\"metadata\":{\"f\":{\"Id\":1234}}}}",
            json);

Copy link
Member Author

Choose a reason for hiding this comment

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

Some bad news here. The OTel SDK will copy/buffer anything exposed by the TState logged if it implements IReadOnlyList or IEnumerable of KeyValuePair<string, object> but ONLY the top-level things. It won't recurse into the values (object?s) of the top-level things and attempt to buffer reference types. I think best to tackle this in the SDK itself?

Copy link
Member

Choose a reason for hiding this comment

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

It won't recurse into the values (object?s) of the top-level things and attempt to buffer reference types. I think best to tackle this in the SDK itself?

I actually don't think the SDK should do anything about these at all. Think about how OS kernels are going to handle interrupts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I'll tackle this separately. This PR isn't creating that problem and trying to solve it here could explode the diff with a lot of unrelated things.

Comment on lines 150 to 154
// Note: For this test we expect the second entry to be dropped because
// the extension goes into "field tracking" mode
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field1", 1));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo", "Hello world"));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field2", 2));
Copy link
Member

Choose a reason for hiding this comment

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

Consider changing the behavior to avoid silent side-effects, perhaps align with what JavaScript is doing.

    state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field1", 1)); // now ext.foo = {"field1": 1}
    state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo", "Hello world")); // now ext.foo = "Hello world"
    state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field2", 2)); // throw exception since "Hello world"["field2"] = 2 is an invalid operation

Copy link
Member Author

Choose a reason for hiding this comment

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

@reyang I just pushed some changes. How the new version works is wholly-owned extensions (the new feature) are limited to maps (IReadOnlyList or IEnumerable of KeyValuePair<string, object?>s) but will be merged with any fields (existing feature) set before or after the map. We could do full JavaScript-y style but I think this is a good balance of features without a whole bunch of cost (complexity & perf). We could always add support for other primitives in the future if the need arises.

Let me know what you think about the new version!

var state = new CommonSchemaJsonSerializationState("Test", writer);

state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext. foo .foo_field1", 1));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo", new List<KeyValuePair<string, object?>> { new KeyValuePair<string, object?>(" foo.field2 ", 2) }));
Copy link
Member

Choose a reason for hiding this comment

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

Should this line overwrite Ln150? If not, what's the rationale? (need to understand the design)

Copy link
Member Author

Choose a reason for hiding this comment

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

The current design is extensions are building up a list of KeyValuePair<string, object>s.

Today you can do this:

state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field1", 1));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field2", 2));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field3", 3));
// output: "ext": { "foo": { "field1": 1, "field2": 2, "field3": 3}  }

After this PR you can do this:

state.AddExtensionAttribute(new KeyValuePair<string, object?>(
   "ext.foo",
   new Dictionary<string, object>() { 
      ["field1"] = 1,
      ["field2"] = 2,
      ["field3"] = 3,
   }));
// output: "ext": { "foo": { "field1": 1, "field2": 2, "field3": 3 } }

If you mix and match these styles, the final list has everything in the order it came in:

state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field1", 1));
state.AddExtensionAttribute(new KeyValuePair<string, object?>(
   "ext.foo",
   new Dictionary<string, object>() { 
      ["field2"] = 2,
      ["field3"] = 3,
   }));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field3", 300));
// output: "ext": { "foo": { "field1": 1, "field2": 2, "field3": 3, "field3": 300 } }

Note "field3" shows up twice in this case.

That is the same behavior as today if you did...

state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field3", 3));
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo.field3", 300));
// output: "ext": { "foo": { "field3":3, "field3": 300 } }

You would also get "field3" twice. There isn't de-duping of extensions because there isn't de-duping of attributes or scopes in logging either. It is just consistently bad across the board. For better or worse! De-duping could be added if users want to pay for the perf, but so far no one is complaining so I just kept it dumb/cheap.

If you try to push something which does not implement IReadOnlyList or IEnumerable of KVP<string, object>...

state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo", "some string");
state.AddExtensionAttribute(new KeyValuePair<string, object?>("ext.foo", 0);

...it will be dropped with a log. We could support these (I did initially), but I decided to limit to ~maps because all the CS extensions currently defined are maps and it greatly reduces the complexity of the logic having the limitation.

Copy link
Member

@reyang reyang 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 concerned about the overall design #1327 (comment), I think we should first agree on what's the right expectation/behavior, then come back to the implementation.

@Kielek
Copy link
Contributor

Kielek commented Aug 31, 2023

@CodeBlanch, changes from this commit cf340f1 are not supported by current dotnat format check. First we should change formatter #1274

@github-actions
Copy link
Contributor

This PR was marked stale due to lack of activity. It will be closed in 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:exporter.onecollector Things related to OpenTelemetry.Exporter.OneCollector Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants