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

[skmonitor] Add monitoring API #201

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

skiplabsdaniel
Copy link
Contributor

No description provided.

@skiplabsdaniel skiplabsdaniel force-pushed the monitoring branch 3 times, most recently from 274f378 to 4c779b9 Compare April 16, 2024 13:20
@gregsexton
Copy link
Contributor

Bear with me here, I do plan to look at this.

Copy link
Contributor

@gregsexton gregsexton 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 great. I really like it.

Big +1 for using open telemetry - the data model (tree of spans, etc) looks solid to me, and we'll be able to utilise all the tools of that ecosystem.

Also, thanks for capturing all the memory info. That will be priceless.

What are your thoughts on the client? Due to the nature of the product, a lot of action is happening there. Should we log at all? Should we just expose to the user or should we try and capture this info somehow? Maybe make it opt in or out?

prelude/runtime/runtime64_specific.cpp Outdated Show resolved Hide resolved
prelude/runtime/obstack.c Outdated Show resolved Hide resolved
prelude/runtime/obstack.c Show resolved Hide resolved
}

fun read_line(): ?String {
.read_line().map(line -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this end up copying?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

Comment on lines +101 to +207
module SKCSV;

fun read_line(): ?String {
SKDB.read_line()
}

fun skipExit<T>(res: Int): T {
SKDB.skipExit(res)
}

fun print_error(error: String): void {
SKDB.print_error(error)
}

fun print_raw(str: String): void {
SKDB.print_raw(str)
}

fun print_string<T: readonly Show>(value: T): void {
SKDB.print_string(value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to capture stuff here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to manage the skipExit for clean exit and readline to get the number of read bytes.
The rest is to be belt and braces.

skmonitor/extern/native.c Outdated Show resolved Hide resolved
Comment on lines 113 to 107
if (!(monitoring() is MOn _)) return void;
SKStore.switchToTrace();
low = get_session_low();
high = get_session_high();
traceId = TraceId::fromHighAndLow(high, low);
optParent = get_span() match {
| Some(parent) ->
saved = SKStore.newObstack();
Some((parent, saved))
| _ ->
initWriter();
None()
};
span = mutable Span(
now(),
traceId,
SpanId::fromInt(Random.next()),
optParent,
);
set_span(Some(span));
SKStore.restoreFromTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put restore in a finally block? Same for elsewhere obviously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, there should never have exceptions during trace calls.

skmonitor/src/Monitor.sk Outdated Show resolved Hide resolved
skmonitor/src/Monitor.sk Show resolved Hide resolved
skmonitor/extern/native.c Outdated Show resolved Hide resolved
@gregsexton
Copy link
Contributor

And just one other thing: we don't have any logging in the production side. So on the server it's SqlTailer.sk and on the client it's the combination of update and diff. Without this we can only see what was processed, but not what was sent.

rfc/007-monitoring.org Outdated Show resolved Hide resolved
@skiplabsdaniel skiplabsdaniel force-pushed the monitoring branch 8 times, most recently from c0909fe to e0de197 Compare April 29, 2024 09:22
Copy link
Contributor

@jberdine jberdine left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple tiny comments

prelude/runtime/runtime64_specific.cpp Outdated Show resolved Hide resolved
prelude/runtime/obstack.c Outdated Show resolved Hide resolved
@skiplabsdaniel skiplabsdaniel force-pushed the monitoring branch 5 times, most recently from 273bfd0 to 127c306 Compare May 1, 2024 09:05
@skiplabsdaniel skiplabsdaniel marked this pull request as ready for review May 2, 2024 07:44
@skiplabsdaniel
Copy link
Contributor Author

On the client side, I think we need to allow it with options for sending data to ours or/and app developer monitoring servers.
We can add the possibility to register the log into IndexedDB if the final user don't allow data sending.

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.

3 participants