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

Split project into modules #39

Merged
merged 4 commits into from
Sep 25, 2022
Merged

Split project into modules #39

merged 4 commits into from
Sep 25, 2022

Conversation

iRevive
Copy link
Contributor

@iRevive iRevive commented Aug 3, 2022

Follow up to #29 (comment).

The new structure is the following:

core:
  core-common
  core-metrics (depends on core-common)
  
testkit:
  testkit-common (depends on core-common)
  testkit-metrics (depends on testkit-common and core-metrics)
  
java:
  java-common (depends on core-common and testkit-common)
  java-metrics (depends on java-common, core-mertics, and testkit-metrics)

That way adding new modules (i.e. tracing, logging) should be easier.

@iRevive iRevive requested a review from rossabaker August 3, 2022 11:19
build.sbt Outdated
Comment on lines 116 to 117
lazy val `java-common` = crossProject(JVMPlatform)
.crossType(CrossType.Pure)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lazy val `java-common` = crossProject(JVMPlatform)
.crossType(CrossType.Pure)
lazy val `java-common` = project

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks better now, thanks

build.sbt Outdated
lazy val `java-common` = crossProject(JVMPlatform)
.crossType(CrossType.Pure)
.in(file("java/common"))
.dependsOn(`core-common`, `testkit-common`)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
.dependsOn(`core-common`, `testkit-common`)
.dependsOn(`core-common`.jvm, `testkit-common`.jvm)

* limitations under the License.
*/

package org.typelevel.otel4s.meta
Copy link
Member

Choose a reason for hiding this comment

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

Should the module name be meta maybe? Typically the module name and package name are somehow matching.

Copy link
Member

Choose a reason for hiding this comment

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

Also, is this file misplaced? Or maybe I'm very confused by the source reorganizaiton 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry, ignore me 😂

build.sbt Outdated
.settings(
name := "otel4s-core-metrics",
libraryDependencies ++= Seq(
"org.typelevel" %%% "cats-effect" % CatsEffectVersion,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need full IO here, should it be ce-kernel or ce-std? Does it matter (it won't if we add fs2 deps). Do we care?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Actually, cats-effect-kernel is enough.

I was thinking about fs2 dependency in tracing module recently. Unless we want to introduce some deep tracing for stream scopes (I'm not even sure how plausible it is) the scodec is more than enough.

Copy link
Member

Choose a reason for hiding this comment

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

I played with extending stream tracing to Natchez, and concluded that having a way to trace Resource was good enough for streams. All a stream dependency brings is a convenience method around Stream.resource(whateverCreatesTheSpan).

Copy link
Contributor Author

@iRevive iRevive Aug 3, 2022

Choose a reason for hiding this comment

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

Yeah, once a streaming scope is described as a Resource, the propagation works well with streams.

def flow(tracer: Tracer[IO]): Stream[IO, Unit] =
for {
span <- Stream.resource(tracer.span("span"))
_ <- Stream.eval(
tracer.currentSpanContext.assertEquals(Some(span.context))
)
span2 <- Stream.resource(tracer.span("span-2"))
_ <- Stream.eval(
tracer.currentSpanContext.assertEquals(Some(span2.context))
)
span3 <- Stream.resource(
tracer.spanBuilder("span-3").withParent(span.context).createAuto
)
_ <- Stream.eval(
tracer.currentSpanContext.assertEquals(Some(span3.context))
)
} yield ()

# Conflicts:
#	build.sbt
@iRevive
Copy link
Contributor Author

iRevive commented Aug 11, 2022

@zmccoy sorry for pinging you personally, but would you be interested in reviewing this PR as well?

There was a discussion regarding the modularity of the project #29 (comment).

And I would prefer to resolve this one before #36.

Copy link
Member

@zmccoy zmccoy 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 great!

@iRevive
Copy link
Contributor Author

iRevive commented Aug 18, 2022

@rossabaker could you have a moment to review this PR, please?

Copy link
Member

@rossabaker rossabaker 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 great to me. I'm sorry I fell into a hole for several weeks.

@@ -20,7 +20,7 @@ import org.typelevel.otel4s.metrics.MeterProvider

trait Otel4s[F[_]] {

/** A registry for creating named [[org.typelevel.otel4s.metrics.Meter]].
/** A registry for creating named meters.
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use a pipe operator if you want it to say meters but link to Meter:

[[org.typelevel.otel4s.metrics.Meter|meters]]

That should not slow this down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doc command was failing. I do not remember the exact reason, but I will bring back the link if it's possible.

Copy link
Member

Choose a reason for hiding this comment

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

@iRevive you should just use a space there, not a |.

@iRevive iRevive merged commit fbfe4f9 into typelevel:main Sep 25, 2022
@iRevive iRevive deleted the modularity branch September 25, 2022 11:18
chr12c pushed a commit to chr12c/otel4s that referenced this pull request Jun 26, 2023
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