Skip to content
This repository has been archived by the owner on Jun 1, 2024. It is now read-only.

Added support for controlling the op_type when indexing #356

Merged
merged 5 commits into from
Sep 18, 2020

Conversation

orjan
Copy link
Contributor

@orjan orjan commented Sep 13, 2020

In order to write to data streams we'll need to set the
op_type to create the default is index

In order to don't serialize null values for the batch action and to not depend
on custom serialization setting we're using LowLevelRequestResponseSerializer.Instance
for serialization of the action.

We won't throw exception when setting the TypeName to null since:

  • This is deprecated in v7
  • Won't work in v8

What issue does this PR address?

Does this PR introduce a breaking change?
I don't think we have introduced an breaking change but we'll need to review the following changes to make sure they're not breaking.

Please check if the PR fulfills these requirements

  • The commit follows our guidelines
  • Unit Tests for the changes have been added (for bug fixes / features)

Other information:

@orjan orjan marked this pull request as draft September 13, 2020 23:32
@mivano
Copy link
Contributor

mivano commented Sep 14, 2020

Thanks for this PR. Looks good. I agree with your points, the exception type does not look like an issue, the other one might be an issue. But that we can highlight in the changes.md and readme.md.

@orjan
Copy link
Contributor Author

orjan commented Sep 14, 2020

@mivano currently there's an issue with the error handling since when we're setting the OpType to create it won't work, since it's looking for index. Also there an open PR #284 trying to address another issue with error.

The question is would we like to stay with the DynamicResponse or are we open to de-serialize the response to DTO:s instead?

@orjan orjan marked this pull request as ready for review September 14, 2020 11:34
@mivano
Copy link
Contributor

mivano commented Sep 16, 2020

Another thing that crossed my mind; the durable part also sends data to ES in a similar manner, but using different code constructs. Most likely the same issue will apply here. For some while now I feel less comfortable with this durable part. It is a large part of the code base with a lot of duplication. And that last part makes introducing these changes more difficult. Besides, the durable functionality is not as stable as it should be and there might be better solutions if you want similar behaviour.

What is your take on this? Worthwhile to apply the same thing to the durable part, exclude it explicitly or something else?

@orjan
Copy link
Contributor Author

orjan commented Sep 16, 2020

Another thing that crossed my mind; the durable part also sends data to ES in a similar manner, but using different code constructs. Most likely the same issue will apply here. For some while now I feel less comfortable with this durable part. It is a large part of the code base with a lot of duplication. And that last part makes introducing these changes more difficult. Besides, the durable functionality is not as stable as it should be and there might be better solutions if you want similar behaviour.

What is your take on this? Worthwhile to apply the same thing to the durable part, exclude it explicitly or something else?

I did a quick fix for the durable part and there are 2 places that needs to be fixed, so I'll fix that as well.
I'm not sure what we should do about the duplicated implementations, in the best of world we should isolate the durable part to a separate lib or at least namespace if possible? Are there any other durable sinks with the same needs?

@mivano
Copy link
Contributor

mivano commented Sep 16, 2020

A totally separate discussion of course from this PR, but I would be inclined to remove it out of this sink and offer functionality in a different way. Preferable using a higher abstraction, so you can specify it on the serilog sinks level instead. This original implementation came from the Seq one (https://github.com/serilog/serilog-sinks-seq/tree/dev/src/Serilog.Sinks.Seq/Sinks/Seq/Durable) so there are more sinks with a similar feature. Problem is that I do not know how many users are actually depending on this durable feature. So it would be a real breaking change with or without another solution.

In order to write to data streams we'll need to set the
op_type to create the default is index

In order to don't serialize null values for the batch action and to not depend
on custom serialization setting we're using LowLevelRequestResponseSerializer.Instance
for serialization of the action.

We won't throw exception when setting the TypeName to null since:
- This is deprecated in v7
- Won't work in v8
In order to be able to use templates and at the same time remove
the doc type we shouldn't override it if it's set to null.
Use the same logic for creating bulk action within
the durable sink as in the standard sink.
@orjan orjan force-pushed the support-for-setting-op-type branch from 4131406 to eeb0a02 Compare September 17, 2020 21:16
@orjan orjan requested a review from mivano September 17, 2020 22:45
@mivano mivano merged commit 144369b into serilog-contrib:dev Sep 18, 2020
@mivano
Copy link
Contributor

mivano commented Sep 18, 2020

Merging it into dev so we have a prerelease package.

orjan added a commit to orjan/serilog-sinks-elasticsearch that referenced this pull request Sep 26, 2020
Fixes a regression introduced in serilog-contrib#356
We should override the TypeName if it's specified.

Note that there are different defaults depending
on how the options are created.
- Created from configuration it defaults to `logevent`
- Created from code it defaults to `_doc`
@orjan orjan mentioned this pull request Sep 26, 2020
2 tasks
mivano pushed a commit that referenced this pull request Sep 28, 2020
Fixes a regression introduced in #356
We should override the TypeName if it's specified.

Note that there are different defaults depending
on how the options are created.
- Created from configuration it defaults to `logevent`
- Created from code it defaults to `_doc`
@orjan orjan deleted the support-for-setting-op-type branch February 3, 2022 09:04
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants