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

[Merged by Bors] - feat(new sink): Initial azure_monitor_logs sink #2811

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - feat(new sink): Initial azure_monitor_logs sink #2811

wants to merge 13 commits into from

Conversation

nazar554
Copy link
Contributor

Solves #1808
Will add validations for configuration and unit tests

@nazar554 nazar554 marked this pull request as ready for review June 13, 2020 17:35
@nazar554
Copy link
Contributor Author

Right now I assume by default that events have timestamp field that is ISO8601. Is there a way to make sure that it's the correct field and it's not using Unix format?

There seems to be an issue when a single event is batched in a JSON array. Azure seems to ignore the timestamp field, maybe because I don't drop milliseconds (although it works fine when there are two or more events)

Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

This looks really good, thanks @nazar554!

Is there some local version of this service that we could add to our integration tests?

We'll also want to add docs, but that can happen in a followup PR.

@lukesteensen
Copy link
Member

Right now I assume by default that events have timestamp field that is ISO8601. Is there a way to make sure that it's the correct field and it's not using Unix format?

By default, events get a timestamp field with an actual Timestamp value, not just a string. But that can often be overwritten with user data that can be a string, so it's hard to guarantee anything. Most sinks will read that field and check the type before using it, which it looks like you're doing.

There seems to be an issue when a single event is batched in a JSON array. Azure seems to ignore the timestamp field, maybe because I don't drop milliseconds (although it works fine when there are two or more events)

That is odd. I didn't see anything that should behave differently with an array of one event.

@nazar554
Copy link
Contributor Author

@lukesteensen

Is there some local version of this service that we could add to our integration tests?

I don't know one, you need to have a Azure Log Analytics service in order to obtain shared_key and customer_id. However, there is free 5 Gb per month plan, so if you have an Azure subscription then it's possible to test this for fee.

That is odd. I didn't see anything that should behave differently with an array of one event.

I think it's the behavior of the Azure Log Analytics. Official documentation suggests a YYYY-MM-DDThh:mm:ssZ date format, but if you actually view some logs on Azure Portal it seems they have milliseconds there. It's possible that docs were not updated.
By default impl Serialize does nanosecond precision and this makes Azure drop the timestamp entirely (it uses the ingestion time instead).
So I added one more commit where I format the date manually and use chrono::SecondsFormat::Millis.

We'll also want to add docs, but that can happen in a followup PR.

I think I can add docs as well, I'll try to use some existing sink as a reference

@jamtur01 jamtur01 added the needs: docs Needs documentation updates label Aug 21, 2020
@jamtur01
Copy link
Contributor

@nazar554 Just thought I'd bump this and see if you have got a moment to re-visit. Thanks! 🥇

@nazar554
Copy link
Contributor Author

@jamtur01 Was a bit busy but I think I can get back to this sometime next week

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
…ay, adjust headers

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
…amp precision

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
@nazar554
Copy link
Contributor Author

@jamtur01 I am not sure how to build & test the docs, should I clone and build timberio/vector-website ?

@jamtur01
Copy link
Contributor

@nazar554 I am not 100% sure how to test off a branch - @binarylogic?

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
@binarylogic binarylogic self-assigned this Aug 23, 2020
@nazar554
Copy link
Contributor Author

Anything else I need to do here? Should I also update vector.spec.toml?

@lukesteensen
Copy link
Member

@nazar554 That file gets updated automatically from the config definitions in .meta/, so you should be good there.

Once @binarylogic is able to help out with testing the docs I'll make another pass and see if there are any remaining blockers. Thanks again for opening this! Excited to get it merged.

@binarylogic
Copy link
Contributor

@nazar554 the docs look good. As long as make check-meta works (which is tested in the CI check workflow), then we're good. No need to do anything with the generated files.

@jamtur01 jamtur01 removed the needs: docs Needs documentation updates label Sep 29, 2020
@lukesteensen
Copy link
Member

@binarylogic it looks like something is up with check-meta here. Is there something that needs to be updated in the docs?

Copy link
Contributor

@jamtur01 jamtur01 left a comment

Choose a reason for hiding this comment

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

One minor issue.


<%= render(
"_partials/fields/_tls_connector_options.toml",
namespace: "sinks.azure_monitor_logs.options",
Copy link
Contributor

@jamtur01 jamtur01 Oct 2, 2020

Choose a reason for hiding this comment

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

This needs:

enabled_default: false,

With the appropriate true or false value. That'll fix the failing Lint suite test.

Copy link
Member

@lukesteensen lukesteensen left a comment

Choose a reason for hiding this comment

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

This is looking good to me! If we can get CI green then I think we're ready to merge.

Thanks for your patience with this @nazar554, and apologies for it lingering so long. We really appreciate the contribution!

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
@nazar554 nazar554 requested a review from jamtur01 October 3, 2020 16:44
Copy link
Contributor

@jamtur01 jamtur01 left a comment

Choose a reason for hiding this comment

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

Looks like some test failures in the CI run.

Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Signed-off-by: Nazar Mishturak <nazarmx@gmail.com>
Copy link
Contributor

@jamtur01 jamtur01 left a comment

Choose a reason for hiding this comment

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

LGTM.

@jamtur01
Copy link
Contributor

jamtur01 commented Oct 3, 2020

@nazar554 I just merged master into the branch because there are some CI changes and I want to make sure the merge goes smooth. Will merge post that.

@jamtur01
Copy link
Contributor

jamtur01 commented Oct 4, 2020

bors merge

bors bot pushed a commit that referenced this pull request Oct 4, 2020
Solves #1808 
Will add validations for configuration and unit tests

Co-authored-by: Luke Steensen <luke.steensen@gmail.com>
Co-authored-by: James Turnbull <james@lovedthanlost.net>
@bors
Copy link

bors bot commented Oct 4, 2020

Pull request successfully merged into master.

Build succeeded:

@bors bors bot changed the title feat(new sink): Initial azure_monitor_logs sink [Merged by Bors] - feat(new sink): Initial azure_monitor_logs sink Oct 4, 2020
@bors bors bot closed this Oct 4, 2020
@nazar554 nazar554 deleted the feature/azure-monitor-logs branch October 4, 2020 13:27
mengesb pushed a commit to jacobbraaten/vector that referenced this pull request Dec 9, 2020
Solves vectordotdev#1808
Will add validations for configuration and unit tests

Co-authored-by: Luke Steensen <luke.steensen@gmail.com>
Co-authored-by: James Turnbull <james@lovedthanlost.net>
Signed-off-by: Brian Menges <brian.menges@anaplan.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants