Skip to content
This repository was archived by the owner on May 21, 2020. It is now read-only.

added SeqApp code#1

Open
DommarajuUmamaheswari wants to merge 52 commits intokmdlogic:devfrom
DommarajuUmamaheswari:dev
Open

added SeqApp code#1
DommarajuUmamaheswari wants to merge 52 commits intokmdlogic:devfrom
DommarajuUmamaheswari:dev

Conversation

@DommarajuUmamaheswari
Copy link
Copy Markdown

Created new SeqApp to send data to Pushgateway
Added files PushgatewayCounterData.cs, SeqAppToPrometheusPushGateway.cs
Based on the signals we select for the events Seq will send those events to SeqApp.
SeqApp will send Prometheus counters for those events to the PushgatewayUrl

Comment thread src/Seq.App.Prometheus.Pushgateway/SeqAppToPrometheusPushGateway.cs Outdated
customGauge.Labels(pushgatewayCounterData.ResourceName).Set(gaugeValue);
}

public static PushgatewayCounterData FormatTemplate(Event<LogEventData> evt, List<string> applicationNameKeyList)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is this method called ‘FormatTemplate’? It doesn’t appear to be doing that...?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ya.. We have changed it to "ApplicationNameKeyValueMapping" now.


public void On(Event<LogEventData> evt)
{
var additionalPropertiesList = SplitOnNewLine(this.ApplicationNameKeySet).ToList();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are these called ‘additionalPropertiesList’?

Copy link
Copy Markdown

@amitxagarwal amitxagarwal Aug 19, 2019

Choose a reason for hiding this comment

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

we can rename it to "applicationNameKeySet".



var customGauge = Metrics.WithCustomRegistry(registry).CreateGauge(GaugeName, "To track the seq errors", new[] { "ApplicationName" });
var gaugeValue = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why would the time of processing be appropriate here? Wouldn’t it better to use a date and time that is part of the actual event, so that we aren’t confused by delays between when the event was created originally and when it was eventually processed into Prometheus?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is a version of Set method which takes timestamp but it never works. So how we do it is we enable the timestamp on the gauge and then set it's value later on with the set method. But when we use this extension method we never get the gauge in the pushgateway.

Also we observed that there are default events in the pushgateway they all follow the same format which is unix time milli seconds.. Not even a single event with date time stamp. So we assumed datetime might not work here.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The question here is about what date and time to use as the starting point, prior to formatting it for the gauge.

Why is DateTimeOffset.UtcNow the most appropriate starting point?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We did have a chat about this, whether we use the seq timestamp as the metric we send rather than the utcnow time. I would expect there to be very little difference between timestamp and utcnow, so either or is ok I guess. It probably makes more sense to go with the seq timestamp - although this could be added as a label on the metric thats pushed as well.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes. We are pushing the Seq Event Timestampinutc as another label in the gauge now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm still struggling to understand why DateTimeOffset.UtcNow is a good value to use here (I also struggle to understand how it's a sensible value generally for any Guage). Sorry if I'm just being slow, it's totally possible I've completely misunderstood something, but I think this is really worth clarifying here.

I think we need to understand the end-to-end use case you're attempting to cover here a bit better. You could start by adding that to the README :) GIve a real-world example of the event you;'d expect to see in Seq, and the corresponding way it would be handled by this Seq app, and finally how it would used on the prometheus side. Would it be graphed and alerted on? What would they look like?

However, while we're here, let's see if I can convey my confusion a little bit better. Let's start with the official documentation from Prometheus about a Guage:

Gauge
A gauge is a metric that represents a single numerical value that can arbitrarily go up and down.

Gauges are typically used for measured values like temperatures or current memory usage, but also "counts" that can go up and down, like the number of concurrent requests.

I understand we could use a guage to "measure" healthy=0, unhealthy=1, and then we can look for any non-zero values and determine what is unhealthy. That makes sense to me.

However, why would we create a guage that always goes up (because generally time always goes forward, the number will only increase)? Why would the guage only go up in response to a particular event in seq? What are we measuring here?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What would make sense to me, is using a Counter that is incremented every time a certain event is found in Seq.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a limitation with pushgateway. We need to compare with a timeframe so we can create alerts etc. Prometheus isn't aware of when metrics are loaded into the pushgateway, so itll just scrape the same metric over and over again for each scrape iteration. You can image then that a single message (say like the current notification emails we have configured in seq prod) gets pushed to pushgateway, how do we know its old? If we include the time in the labels, then a counter is redundant anyhow. Every new message will create a time series and hence start a new counter.

We could perhaps investigate using the metric in combination with the last_pushed_time_seconds as per below, then move back to using a counter?

Push_time_seconds

var pushgatewayCounterData = FormatTemplate(evt, additionalPropertiesList);


var customGauge = Metrics.WithCustomRegistry(registry).CreateGauge(GaugeName, "To track the seq errors", new[] { "ApplicationName" });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why is “ApplicationName” here?

Copy link
Copy Markdown
Collaborator

@adamchester adamchester Aug 16, 2019

Choose a reason for hiding this comment

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

“To track the Seq errors” is a little confusing to me. What’s this description used for, and why is it only errors? Couldn’t this Seq app also send any events (not just errors) to Prometheus ?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Got it. Will be renaming this to "To track the Seq events based on the applied signal".

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ApplicationName is to track track events by Application in the seq.. If not ApplicationName then what else this could be?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

we have made it as an input from UI now.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I still don’t understand how ApplicationName is being used, could you explain further please?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ok I got it now, it’s configuration now, as the Guatemalan name.

{
if (property.Key == name)
{
data.ResourceName = property.Value.ToString();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why do we have potentially multiple properties, but then we always overwrite whatever value was previously collected? It looks like we only keep a single value here, so why have the list?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The reason for this logic here is in our seq logs we have lot of events which contains Application name to which the events belongs with the Key as Application as well as App. Now these two we have noticed till now. Not sure in how many more such keys are there cause if you see in this code we are defaulting the resource name as "ResourceNotFOund" and in pushgateway sometimes we get this as well.


[SeqAppSetting(
DisplayName = "ApplicationNameKeyList",
HelpText = "The names of additional event properties.",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This description seems unclear, I could not guess what values might be required simply by reading this.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This we have changed it a while back

public string ApplicationNameKeySet { get; set; }

public IMetricPushServer server;
public readonly string instanceName = "default";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why bother making this an instance member variable? Wouldn’t a constant be clearer?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Have removed the instanceName

namespace Seq.App.Prometheus.Pushgateway
{
[SeqApp("Seq.App.Prometheus.Pushgateway",
Description = "Filtered events are sent to the Prometheus Pushgateway.")]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It doesn’t seem like the events are being sent to Prometheus push gateway. It seems like a counter is being sent each time the event is processed. Have I misunderstood something?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

We have changed the description as we are "GaugeLabelKey data from filtered events are sent through gauge to the Pushgateway."

data.RenderedMessage = evt.Data.RenderedMessage ?? evt.Data.MessageTemplate;
foreach (var property in properties)
{
if (property.Key == "Application" || property.Key == "App")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should allow this to be customizable as part of configuration - This is too specific

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is not there now. It is outdated. We have already made it configurable.



var customGauge = Metrics.WithCustomRegistry(registry).CreateGauge(GaugeName, "To track the seq errors", new[] { "ApplicationName" });
var gaugeValue = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We did have a chat about this, whether we use the seq timestamp as the metric we send rather than the utcnow time. I would expect there to be very little difference between timestamp and utcnow, so either or is ok I guess. It probably makes more sense to go with the seq timestamp - although this could be added as a label on the metric thats pushed as well.

public void On(Event<LogEventData> evt)
{
var gaugeLabelValuesList = SplitOnNewLine(this.GaugeLabelValues).ToList();
var pushgatewayCounterData = ApplicationNameKeyValueMapping(evt, gaugeLabelValuesList);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not really a counter anymore right? Perhaps this needs a rename

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

THis is also fixed now.

Comment thread README.md Outdated
* Fill the details `Title`
* Check the box `Stream incoming events` to send events and select the signal
* Fill in the details `Pushgateway URL`, `Pushgateway Gauge Name`, `Pushgateway Gauge Label Key`, `Pushgateway Gague Label Values`
* click on `SAVE CHANGES`
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Avoid listing the generic steps to configure a Seq app. Only list those things that are specific to this Seq app. Above there is a link to https://docs.getseq.net/docs/installing-seq-apps which is the official documentation and will be maintained well in the future, so we can just leverage that one.

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.

4 participants