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

Logicmonitorexporter implementation #14182

Closed
wants to merge 9 commits into from
Closed

Logicmonitorexporter implementation #14182

wants to merge 9 commits into from

Conversation

khyatigandhi6
Copy link
Contributor

Description:
Adding new component logicmonitorexporter for exporting traces and logs to Logicmonitor.

Link to tracking Issue: #13727

Testing: Added unit test cases.

Documentation: Details are mentioned in README.md file.

@lalit-fgr
Copy link

@khyatigandhi6 can you please resolve merge conflicts

@lalit-fgr
Copy link

@Aneurysm9 Can you please review this

@TylerHelmuth
Copy link
Member

@dmitryax are you the rotating sponsor for this component?

@khyatigandhi6
Copy link
Contributor Author

Hi @Aneurysm9 ,
Can you please assign this PR to the sponsor of this component for reviewing ?

@Aneurysm9
Copy link
Member

Who is the sponsor of this component? I still see #13727 tagged as needing a sponsor.

@jpkrohling
Copy link
Member

@bogdandrutu, it looks like you are the assigned sponsor for this one.

@lalit-fgr
Copy link

@khyatigandhi6 can you please resolve the merge conflicts
@bogdandrutu a gentle reminder, if you can please this PR.

.github/ALLOWLIST Outdated Show resolved Hide resolved
@khyatigandhi6
Copy link
Contributor Author

Hi Team,
Updated the PR by resolving merge conflicts. Could you please review it?

@khyatigandhi6 khyatigandhi6 requested review from mx-psi and removed request for dashpole, djaglowski and TylerHelmuth November 1, 2022 04:10
@mx-psi mx-psi requested a review from bogdandrutu November 2, 2022 09:19
mx-psi
mx-psi previously approved these changes Nov 7, 2022
@mx-psi mx-psi dismissed their stale review November 7, 2022 10:40

sorry, didn't mean to approve this

@mx-psi
Copy link
Member

mx-psi commented Nov 7, 2022

Removing the 'request changes' since it was addressed. @bogdandrutu this is on you as the sponsor :)

@github-actions github-actions bot added the cmd/configschema configschema command label Nov 11, 2022
@github-actions github-actions bot requested review from mx-psi and pmcollins November 11, 2022 06:30
@lalit-fgr
Copy link

@bogdandrutu can we please get some 👀 to it.

Copy link
Member

@bogdandrutu bogdandrutu left a comment

Choose a reason for hiding this comment

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

This needs more work and consider to split in simpler PRs to review.

Comment on lines +17 to 19
require github.com/logicmonitor/lm-data-sdk-go v0.5.0 // indirect

require (
Copy link
Member

Choose a reason for hiding this comment

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

Same block please

@@ -0,0 +1 @@
include ../../Makefile.Common
Copy link
Member

Choose a reason for hiding this comment

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

New line.

Comment on lines +29 to +32
// LMAuthenticator is used for authenticating requests to Logicmonitor platform
type LMAuthenticator struct {
Config *Config
}
Copy link
Member

Choose a reason for hiding this comment

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

Why public? Who is using this?

Comment on lines +42 to +43
// URL on which the logs/traces should be forwarded
URL string `mapstructure:"url"`
Copy link
Member

Choose a reason for hiding this comment

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

confighttp.HTTPClientSettings already has notion of "endpoint" this is confusing.

Comment on lines +45 to +47
// Headers for HTTP requests
Headers map[string]string `mapstructure:"headers"`

Copy link
Member

Choose a reason for hiding this comment

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

@bogdandrutu
Copy link
Member

@khyatigandhi6
Copy link
Contributor Author

Thanks @bogdandrutu for the review comments!
I will break down this PR into 3 parts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmd/configschema configschema command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants