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

New central config options: CaptureHeaders, LogLevel, SpanFramesMinDuration, StackTraceLimit #794

Closed
gregkalapos opened this issue Mar 27, 2020 · 5 comments · Fixed by #820
Assignees
Milestone

Comments

@gregkalapos
Copy link
Contributor

gregkalapos commented Mar 27, 2020

These 4 configs are not yet supported by .NET, but they are already added in Kibana.

Given we already have the infrastructure, let's add these 4 and make them also runtime changeable.

capture_headers
log_level
span_frames_min_duration
stack_trace_limit
@gregkalapos
Copy link
Contributor Author

Note to future self: Don't forget to update the docs ;)

@gregkalapos
Copy link
Contributor Author

Big progress was made on this by @vhatsura in #820 👏

Missing part to close this:
LogLevel does not work yet.

  • We should make sure our loggers read the value that we fetch from central config
  • We should also think about how this integrates with .NET Core logging (e.g. do we really want to override the log level defined in appsettings.json or elsewhere, etc.)

@gregkalapos gregkalapos reopened this Apr 28, 2020
@vhatsura vhatsura mentioned this issue May 9, 2020
2 tasks
@vhatsura
Copy link
Contributor

vhatsura commented May 9, 2020

@gregkalapos,
(I've tried to resolve it, at least, for console logger, but it instantiates before remote configuration and used by central config fetcher itself. Looks like we need to add a new method on IApmLogger interface, smth like SetLogLevel. WDYT?
As for

We should also think about how this integrates with .NET Core logging

I'd say, we don't need to do it, due to log level is defined on common logging configuration.

@gregkalapos
Copy link
Contributor Author

@gregkalapos,
(I've tried to resolve it, at least, for console logger, but it instantiates before remote configuration and used by central config fetcher itself. Looks like we need to add a new method on IApmLogger interface, smth like SetLogLevel. WDYT?
As for

We should also think about how this integrates with .NET Core logging

I'd say, we don't need to do it, due to log level is defined on common logging configuration.

Looks like we need to add a new method on IApmLogger interface, smth like SetLogLevel. WDYT?

Well, it'd be really nice to avoid it - otherwise all implementors need to implement it - and I know for our IIS module it's common that people redirect logs by implementing IApmLogger - for them this would mean a build error. This would be especially less nice if all the benefit would be that our Console Logger uses it, but e.g. in ASP.NET Core we ignore this.

@gregkalapos gregkalapos modified the milestones: 7.7, 7.8 May 28, 2020
@gregkalapos gregkalapos modified the milestones: 7.8, 7.9 Jul 9, 2020
@russcam russcam modified the milestones: 7.9, 7.10 Oct 27, 2020
@gregkalapos gregkalapos modified the milestones: 7.10, 7.11 Nov 10, 2020
@gregkalapos
Copy link
Contributor Author

With #1096 this is done.

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 a pull request may close this issue.

3 participants